Skip to content

TEZ-4729: Improve ShuffleHandler - #512

Merged
ayushtkn merged 2 commits into
apache:masterfrom
ayushtkn:TEZ-4729
Jun 25, 2026
Merged

TEZ-4729: Improve ShuffleHandler#512
ayushtkn merged 2 commits into
apache:masterfrom
ayushtkn:TEZ-4729

Conversation

@ayushtkn

Copy link
Copy Markdown
Member

No description provided.

@tez-yetus

This comment was marked as outdated.

@tez-yetus

This comment was marked as outdated.

@tez-yetus

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeLogfileComment
+0 🆗reexec0m 11sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+0 🆗detsecrets0m 0sdetect-secrets was not available.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 1 new or modified test files.
_ master Compile Tests _
+0 🆗mvndep0m 36sMaven dependency ordering for branch
+1 💚mvninstall4m 39smaster passed
+1 💚compile4m 3smaster passed
+1 💚checkstyle1m 9smaster passed
+1 💚javadoc0m 58smaster passed
+0 🆗spotbugs1m 41stez-dag in master has 537 extant spotbugs warnings.
+0 🆗spotbugs0m 46stez-plugins/tez-aux-services in master has 5 extant spotbugs warnings.
_ Patch Compile Tests _
+0 🆗mvndep0m 10sMaven dependency ordering for patch
+1 💚mvninstall3m 52sthe patch passed
+1 💚codespell1m 36sNo new issues.
+1 💚compile4m 4sthe patch passed
+1 💚javac4m 4sthe patch passed
+1 💚blanks0m 0sThe patch has no blanks issues.
-0 ⚠️checkstyle0m 40s/results-checkstyle-tez-dag.txttez-dag: The patch generated 1 new + 187 unchanged - 1 fixed = 188 total (was 188)
+1 💚javadoc0m 57sthe patch passed
+1 💚spotbugs2m 54sthe patch passed
_ Other Tests _
+1 💚unit74m 48sroot in the patch passed.
+1 💚asflicense0m 59sThe patch does not generate ASF License warnings.
106m 30s
SubsystemReport/Notes
DockerClientAPI=1.55 ServerAPI=1.55 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-512/5/artifact/out/Dockerfile
Optional Testsdupname compile unit asflicense javac javadoc spotbugs checkstyle codespell detsecrets
unameLinux dc444b35b4a8 5.15.0-181-generic #191-Ubuntu SMP Fri May 22 19:09:02 UTC 2026 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitytez-personality.sh
git revisionmaster / 61cc636
Default JavaEclipse Adoptium-21.0.11+10-LTS
Test Resultshttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-512/5/testReport/
Max. process+thread count2075 (vs. ulimit of 5500)
modulesC: tez-dag tez-plugins/tez-aux-services U: .
Console outputhttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-512/5/console
versionsgit=2.43.0 maven=3.9.15 spotbugs=4.9.3 codespell=2.4.1
Powered byApache Yetus 0.15.1 https://yetus.apache.org

This message was automatically generated.

// Create a JobTokenSecretManager initialized with the session token's password
// for shuffle delete requests. The shuffle handler verifies requests using the
// token password, so the MAC must be initialized with it (as tasks do).
shuffleJobTokenSecretManager = new JobTokenSecretManager(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not sure if we need a separate secret manager, as the sessionToken is provided for jobTokenSecretManager already:

jobTokenSecretManager.addTokenForJob(

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

JobTokenSecretManager has two different key paths:

  1. computeHash(byte[] msg) (instance method) — uses mac.doFinal(msg) with the MAC master key (random at construction)
  2. retrieveTokenSecret(String jobId) — returns the per-job key from the currentJobTokens map (populated by addTokenForJob)

The client-side AsyncHttpConnection.computeEncHash() calls:
encHash = SecureShuffleUtils.hashFromString(msgToEncode, jobTokenSecretMgr);
// → mgr.computeHash(msg) ← uses MAC master key, NOT the per-job token from the map

The server-side verifyRequest calls:
SecretKey tokenSecret = getSecretManager().retrieveTokenSecret(appid); // ← uses the map
SecureShuffleUtils.verifyReply(urlHashStr, enc_str, tokenSecret);

So addTokenForJob populates the map (server path), but the client hash computation uses the MAC field (instance method). They're completely separate code paths. The MAC must be initialized
with the token password for the client-side hash to match what the server verifies.

It was leading to test failure here: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-512/3/testReport/org.apache.tez.auxservices/TestShuffleHandlerJobs/testOrderedWordCount/

Did I decode it wrong?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

that's right, thanks for clarifying, I tend to forget SecretManager internals
so the code comment indeed explains the situation well: client side uses the instance method, so we must initialize the instance MAC with the sessionToken, which is used from the map on the server/shufflehandler side

@ayushtkn
ayushtkn merged commit c334e94 into apache:masterJun 25, 2026
6 checks passed
Sign up for freeto 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

@ayushtkn@tez-yetus@abstractdog