Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HIVE-28802: NPE in MiniHS2 with miniHS2.clusterType=LOCALFS_ONLY #5680

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

abstractdog
Copy link
Contributor

@abstractdog abstractdog commented Mar 5, 2025

What changes were proposed in this pull request?

Removed an mkdir from MiniHS2 initialization.

Why are the changes needed?

I didn't find why it was needed (introduced in the original MiniHS2 patch), and the whole thing works without it (but caused an NPE in LOCALFS_ONLY mode.

Does this PR introduce any user-facing change?

No.

Is the change a dependency upgrade?

No.

How was this patch tested?

Worked with LLAP/TEZ/LOCALFS_ONLY, ran a simple query that needs tez session (I was assuming that the staging dir hack I removed was needed for tez apps):

CREATE TABLE test_part(id int) PARTITIONED BY(dt string) STORED AS ORC;
INSERT INTO test_part VALUES (1, '1'),(2, '2');

locafs_only

mvn clean install -Dtest=StartMiniHS2Cluster -DminiHS2.conf="target/testconf/llap/hive-site.xml"  -DminiHS2.run=true -DminiHS2.usePortsFromConf=true -pl itests/hive-unit -Pitests -DminiHS2.clusterType=LOCALFS_ONLY

llap

mvn clean install -Dtest=StartMiniHS2Cluster -DminiHS2.conf="target/testconf/llap/hive-site.xml"  -DminiHS2.run=true -DminiHS2.usePortsFromConf=true -pl itests/hive-unit -Pitests -DminiHS2.clusterType=LLAP

tez

mvn clean install -Dtest=StartMiniHS2Cluster -DminiHS2.run=true -DminiHS2.usePortsFromConf=true -pl itests/hive-unit -Pitests -DminiHS2.clusterType=TEZ

sanity-checked itests/hive-unit/target/tmp/log/hive.log but haven't found any suspicious exceptions

@kokila-19
Copy link
Contributor

Was just curious as you said all clusterType seems to work without this and checked in code to find there are multiple test files where this directory is created.
I tested locally by removing the creation of directory in

miniHS2.getDFS().getFileSystem().mkdirs(new Path("/apps_staging_dir/anonymous"));
and the test passed.

@kokila-19
Copy link
Contributor

The test files that has the directory

TestWMMetricsWithTrigger - Ignored test
TestTriggersMoveWorkloadManager - Ignored test
TestKillQueryWithAuthorizationDisabled - Ignored test
TestTriggersWorkloadManager - Ignored test

BaseJdbcWithMiniLlap
AbstractJdbcTriggersTest
AbstractTestJdbcGenericUDTFGetSplits
TestReExecuteKilledTezAMQueryPlugin

Maybe we could check if at all this directory is actually being used at some point in these files.

@abstractdog
Copy link
Contributor Author

The test files that has the directory

TestWMMetricsWithTrigger - Ignored test TestTriggersMoveWorkloadManager - Ignored test TestKillQueryWithAuthorizationDisabled - Ignored test TestTriggersWorkloadManager - Ignored test

BaseJdbcWithMiniLlap AbstractJdbcTriggersTest AbstractTestJdbcGenericUDTFGetSplits TestReExecuteKilledTezAMQueryPlugin

Maybe we could check if at all this directory is actually being used at some point in these files.

good point @kokila-19 , let me check the unit tests and remove the same where it's possible

@abstractdog
Copy link
Contributor Author

removed this path from all unit tests, enabled them temporarily and tested, all works, looks like this was a copied code for no reason

 mvn install -Dtest.groups= -DfailIfNoTests=true -Dtest.output.overwrite=true -Pitests,iceberg -Denforcer.skip=true -Drat.skip=true -nsu -Dtest=TestTriggersMoveWorkloadManager,TestTriggersNoTezSessionPool,TestTriggersTezSessionPoolManager,TestTriggersWorkloadManager -pl ./itests/hive-unit

 mvn install -Dtest.groups= -DfailIfNoTests=true -Dtest.output.overwrite=true -Pitests,iceberg -Denforcer.skip=true -Drat.skip=true -nsu -Dtest=TestJdbcWithMiniLlapRow,TestJdbcWithMiniLlapVectorArrow,TestJdbcWithMiniLlapVectorArrowBatch,TestLlapExtClientWithCloudDeploymentConfigs,TestMiniLlapVectorArrowWithLlapIODisabled,TestNewGetSplitsFormat,TestNewGetSplitsFormatReturnPath -pl ./itests/hive-unit

@kokila-19
Copy link
Contributor

LGTM +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants