Skip to content

Conversation

@abstractdog
Copy link
Contributor

@abstractdog abstractdog commented Sep 22, 2025

See: jira description

Additional local testing:

  1. removed the method
  2. rebuilt tez 0.10.5
  3. ran hive on tez unit test locally:
mvn install -Dtest.output.overwrite=true -Pitests -nsu -pl itests/qtest -Dtest=TestMiniTezCliDriver -Dqfile=acid_vectorization_original_tez.q

Also tez has unit test coverage in this area: in case it's green, this patch is safe.

@tez-yetus

This comment was marked as outdated.

@tez-yetus

This comment was marked as outdated.

@abstractdog
Copy link
Contributor Author

abstractdog commented Sep 23, 2025

apparently, there is a failing case:

[ERROR]   TestDAGAppMaster.testParseAllPluginsOnlyCustomSpecified:275->verifyDescAndMap:413 expected:<2> but was:<1>

I'm going to investigate this: it seems like this unit test asserts that the yarn plugin descriptor is hacked to the entities even when enableYarn:false, so the application expects it to be present in non-local mode, even when containersEnabled=false...which still seems quite hacky, need to understand what use case this unit test covers

EDIT: in the only 1 known custom plugin use-case, which is Hive LLAP, Hive sets the descriptor to enableYarn:true:
https://github.com/apache/hive/blob/c338904b80d4dc6df08981738b69fa136bf6c624/ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java#L341
because this calls enableContainers=true case:

return new ServicePluginsDescriptor(true, enableUber, null, null, null);

so, this unit test doesn't cover a known use-case
I'm about to remove it or change the asserted values with an explanation in a code comment

@tez-yetus

This comment was marked as outdated.

@tez-yetus
Copy link

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 39s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 detsecrets 0m 0s detect-secrets was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 1 new or modified test files.
_ master Compile Tests _
+1 💚 mvninstall 13m 49s master passed
+1 💚 compile 0m 47s master passed
+1 💚 checkstyle 1m 19s master passed
+1 💚 javadoc 0m 37s master passed
+0 🆗 spotbugs 2m 35s tez-dag in master has 785 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚 mvninstall 0m 31s the patch passed
+1 💚 codespell 0m 26s No new issues.
+1 💚 compile 0m 30s the patch passed
+1 💚 javac 0m 30s the patch passed
+1 💚 blanks 0m 1s The patch has no blanks issues.
+1 💚 checkstyle 0m 16s the patch passed
+1 💚 javadoc 0m 14s the patch passed
+1 💚 spotbugs 1m 45s the patch passed
_ Other Tests _
+1 💚 unit 5m 11s tez-dag in the patch passed.
+1 💚 asflicense 0m 14s The patch does not generate ASF License warnings.
30m 9s
Subsystem Report/Notes
Docker ClientAPI=1.51 ServerAPI=1.51 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-433/4/artifact/out/Dockerfile
GITHUB PR #433
Optional Tests dupname asflicense javac javadoc unit spotbugs checkstyle codespell detsecrets compile
uname Linux eda0a00b806e 5.15.0-156-generic #166-Ubuntu SMP Sat Aug 9 00:02:46 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality /home/jenkins/jenkins-home/workspace/tez-multibranch_PR-433/src/.yetus/personality.sh
git revision master / d5bfce2
Default Java Ubuntu-21.0.8+9-Ubuntu-0ubuntu124.04.1
Test Results https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-433/4/testReport/
Max. process+thread count 221 (vs. ulimit of 5500)
modules C: tez-dag U: tez-dag
Console output https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-433/4/console
versions git=2.43.0 maven=3.8.7 spotbugs=4.9.3 codespell=2.0.0
Powered by Apache Yetus 0.15.1 https://yetus.apache.org

This message was automatically generated.

Copy link
Member

@ayushtkn ayushtkn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@abstractdog abstractdog merged commit 8be8008 into apache:master Oct 13, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants