Skip to content

Conversation

abstractdog
Copy link
Contributor

@abstractdog abstractdog commented Feb 19, 2025

This PR makes the JobTokenSecretManager in Tez able to use the algorithm specified in:

hadoop.security.secret-manager.key-generator.algorithm

An additional comment was needed to upgrade to hadoop 3.5.0-SNAPSHOT to compile against the changes of YARN-11738. This PR can be merged once Tez upgraded its hadoop.version to 3.5.0 (which needs a hadoop release first :) )

testing done: turned on SSL shuffle on a cluster having YARN-11738, DAG failed with exceptions mentioned on jira, then with the fix DAG ran successfully (as ShuffleScheduler + ShuffleManager used the non-default algorithm configured)

@tez-yetus

This comment was marked as outdated.

@abstractdog abstractdog changed the title TEZ-4600: JobTokenSecretManager should respect the algorithm set by hadoop TEZ-4600: Secret managers in Tez should respect the algorithm set by hadoop Feb 25, 2025
@abstractdog abstractdog requested a review from ayushtkn February 25, 2025 09:30
} catch (NoSuchAlgorithmException nsa) {
throw new IllegalArgumentException("Can't find " + algorithm + " algorithm.", nsa);
} catch (InvalidKeyException ike) {
throw new IllegalArgumentException("Invalid key to HMAC computation", ike);
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to hardcode HMAC over here? InvalidKeyException can be thrown for other algorithms as well, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, right, this is a leftover from the default HMAC-era

@tez-yetus

This comment was marked as outdated.

@tez-yetus
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 38s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 1s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 10 new or modified test files.
_ master Compile Tests _
+0 🆗 mvndep 2m 51s Maven dependency ordering for branch
+1 💚 mvninstall 9m 44s master passed
+1 💚 compile 5m 45s master passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 compile 5m 24s master passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+1 💚 checkstyle 3m 58s master passed
+1 💚 javadoc 4m 7s master passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 javadoc 3m 50s master passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+0 🆗 spotbugs 5m 47s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 11m 29s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 57s Maven dependency ordering for patch
+1 💚 mvninstall 7m 11s the patch passed
+1 💚 compile 6m 6s the patch passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 javac 6m 6s the patch passed
+1 💚 compile 5m 39s the patch passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+1 💚 javac 5m 39s the patch passed
+1 💚 checkstyle 0m 28s tez-api: The patch generated 0 new + 41 unchanged - 1 fixed = 41 total (was 42)
+1 💚 checkstyle 0m 31s The patch passed checkstyle in tez-runtime-library
+1 💚 checkstyle 0m 25s The patch passed checkstyle in tez-mapreduce
+1 💚 checkstyle 0m 35s The patch passed checkstyle in tez-dag
+1 💚 checkstyle 0m 24s The patch passed checkstyle in tez-ext-service-tests
+1 💚 checkstyle 0m 23s The patch passed checkstyle in tez-aux-services
+1 💚 checkstyle 0m 58s root: The patch generated 0 new + 329 unchanged - 1 fixed = 329 total (was 330)
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 xml 0m 2s The patch has no ill-formed XML file.
+1 💚 javadoc 3m 56s the patch passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 javadoc 3m 52s the patch passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+1 💚 findbugs 12m 23s the patch passed
_ Other Tests _
+1 💚 unit 0m 45s tez-api in the patch passed.
+1 💚 unit 0m 36s tez-runtime-library in the patch passed.
+1 💚 unit 0m 34s tez-mapreduce in the patch passed.
+1 💚 unit 0m 41s tez-dag in the patch passed.
+1 💚 unit 0m 35s tez-ext-service-tests in the patch passed.
+1 💚 unit 0m 33s tez-aux-services in the patch passed.
+1 💚 unit 3m 17s root in the patch passed.
+1 💚 asflicense 2m 38s The patch does not generate ASF License warnings.
105m 9s
Subsystem Report/Notes
Docker ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-393/3/artifact/out/Dockerfile
GITHUB PR #393
Optional Tests dupname asflicense javac javadoc unit xml compile spotbugs findbugs checkstyle
uname Linux 64af6b038ae0 5.15.0-130-generic #140-Ubuntu SMP Wed Dec 18 17:59:53 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/tez.sh
git revision master / 82b3476
Default Java Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
Test Results https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-393/3/testReport/
Max. process+thread count 101 (vs. ulimit of 5500)
modules C: tez-api tez-runtime-library tez-mapreduce tez-dag tez-ext-service-tests tez-plugins/tez-aux-services . U: .
Console output https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-393/3/console
versions git=2.34.1 maven=3.6.3 findbugs=3.0.1
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@abstractdog
Copy link
Contributor Author

tests passed
let me know @ayushtkn if this can be approved
we cannot merge until Hadoop 3.5.0, created TEZ-4606 for blocking this jira

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

@tez-yetus
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 25m 6s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 1s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 12 new or modified test files.
_ master Compile Tests _
+0 🆗 mvndep 2m 0s Maven dependency ordering for branch
+1 💚 mvninstall 10m 19s master passed
+1 💚 compile 3m 34s master passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 compile 3m 20s master passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+1 💚 checkstyle 2m 48s master passed
+1 💚 javadoc 2m 57s master passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 javadoc 2m 43s master passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+0 🆗 spotbugs 0m 46s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 5m 57s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 10s Maven dependency ordering for patch
+1 💚 mvninstall 2m 18s the patch passed
+1 💚 compile 2m 19s the patch passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 javac 2m 19s the patch passed
+1 💚 compile 2m 6s the patch passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+1 💚 javac 2m 6s the patch passed
-0 ⚠️ checkstyle 0m 13s tez-api: The patch generated 1 new + 41 unchanged - 1 fixed = 42 total (was 42)
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 javadoc 1m 32s the patch passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 javadoc 1m 29s the patch passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+1 💚 findbugs 5m 23s the patch passed
_ Other Tests _
+1 💚 unit 2m 16s tez-api in the patch passed.
+1 💚 unit 5m 47s tez-runtime-library in the patch passed.
+1 💚 unit 1m 23s tez-mapreduce in the patch passed.
+1 💚 unit 5m 5s tez-dag in the patch passed.
+1 💚 unit 4m 32s tez-ext-service-tests in the patch passed.
+1 💚 unit 2m 50s tez-aux-services in the patch passed.
+1 💚 asflicense 0m 55s The patch does not generate ASF License warnings.
100m 56s
Subsystem Report/Notes
Docker ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-393/4/artifact/out/Dockerfile
GITHUB PR #393
Optional Tests dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname Linux a4dd1a21cd29 5.15.0-131-generic #141-Ubuntu SMP Fri Jan 10 21:18:28 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/tez.sh
git revision master / 82b3476
Default Java Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
checkstyle https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-393/4/artifact/out/diff-checkstyle-tez-api.txt
Test Results https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-393/4/testReport/
Max. process+thread count 2100 (vs. ulimit of 5500)
modules C: tez-api tez-runtime-library tez-mapreduce tez-dag tez-ext-service-tests tez-plugins/tez-aux-services U: .
Console output https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-393/4/console
versions git=2.34.1 maven=3.6.3 findbugs=3.0.1
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@abstractdog
Copy link
Contributor Author

changed hadoop enums to strings until TEZ-4607
merging this soon assuming that @ayushtkn 's approval still holds

@ayushtkn
Copy link
Member

ayushtkn commented Mar 3, 2025

there is a checkstyle warning that has shown up for the unused import, we need to fix it before we merge

@abstractdog
Copy link
Contributor Author

there is a checkstyle warning that has shown up for the unused import, we need to fix it before we merge

makes sense, I just pushed a commit to remove it

@tez-yetus
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 25m 44s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 12 new or modified test files.
_ master Compile Tests _
+0 🆗 mvndep 2m 5s Maven dependency ordering for branch
+1 💚 mvninstall 9m 45s master passed
+1 💚 compile 3m 30s master passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 compile 3m 13s master passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+1 💚 checkstyle 2m 43s master passed
+1 💚 javadoc 2m 39s master passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 javadoc 2m 29s master passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+0 🆗 spotbugs 0m 41s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 5m 29s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 9s Maven dependency ordering for patch
+1 💚 mvninstall 2m 11s the patch passed
+1 💚 compile 2m 7s the patch passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 javac 2m 7s the patch passed
+1 💚 compile 1m 56s the patch passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+1 💚 javac 1m 56s the patch passed
+1 💚 checkstyle 0m 12s tez-api: The patch generated 0 new + 41 unchanged - 1 fixed = 41 total (was 42)
+1 💚 checkstyle 0m 16s The patch passed checkstyle in tez-runtime-library
+1 💚 checkstyle 0m 10s The patch passed checkstyle in tez-mapreduce
+1 💚 checkstyle 0m 19s The patch passed checkstyle in tez-dag
+1 💚 checkstyle 0m 10s The patch passed checkstyle in tez-ext-service-tests
+1 💚 checkstyle 0m 10s The patch passed checkstyle in tez-aux-services
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 javadoc 1m 22s the patch passed with JDK Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04
+1 💚 javadoc 1m 21s the patch passed with JDK Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
+1 💚 findbugs 5m 1s the patch passed
_ Other Tests _
+1 💚 unit 2m 11s tez-api in the patch passed.
+1 💚 unit 6m 17s tez-runtime-library in the patch passed.
+1 💚 unit 1m 21s tez-mapreduce in the patch passed.
+1 💚 unit 5m 1s tez-dag in the patch passed.
+1 💚 unit 4m 43s tez-ext-service-tests in the patch passed.
+1 💚 unit 2m 51s tez-aux-services in the patch passed.
+1 💚 asflicense 0m 53s The patch does not generate ASF License warnings.
98m 44s
Subsystem Report/Notes
Docker ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-393/5/artifact/out/Dockerfile
GITHUB PR #393
Optional Tests dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname Linux c3870ff10f60 5.15.0-131-generic #141-Ubuntu SMP Fri Jan 10 21:18:28 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/tez.sh
git revision master / 82b3476
Default Java Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.26+4-post-Ubuntu-1ubuntu122.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_442-8u442-b06us1-0ubuntu122.04-b06
Test Results https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-393/5/testReport/
Max. process+thread count 1534 (vs. ulimit of 5500)
modules C: tez-api tez-runtime-library tez-mapreduce tez-dag tez-ext-service-tests tez-plugins/tez-aux-services U: .
Console output https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-393/5/console
versions git=2.34.1 maven=3.6.3 findbugs=3.0.1
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@ayushtkn ayushtkn merged commit 3aa2aa1 into apache:master Mar 4, 2025
6 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