HADOOP-19212. Hadoop UGI compatible with Java 25 #7886
Merged
+652
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I think it would be a good idea to initiate a discussion on this implementation on the hadoop-common dev list, or at least ping everyone here who commented on the orignal PRs so that we can get a consensus on this. Of course this looks good to me. Also could you add a "co-authored-by" line so that I show up in the github statistics ? |
|
@stoty thanks for your suggestion. I updated the PR description with the "co-authored-by" sections. Since the |
|
Thanks @pan3793 , I have done some verification in the production cluster to confirm that this PR works. |
|
💔 -1 overall
This message was automatically generated. |
|
Spotted an issue during self-review, the thrown exception seems to have an additional wrapper on JDK 25, related test cases Updated: |
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Pull Request Overview
This PR updates Hadoop's UserGroupInformation (UGI) to be compatible with Java 25 by replacing deprecated Subject methods with a new compatibility utility. The deprecated Subject.getSubject(AccessControlContext) and Subject.doAs() methods have been removed in Java 24+, requiring a migration to Subject.current() and Subject.callAs().
- Introduces a new
SubjectUtilclass that provides compatibility between old and new Subject APIs - Updates UGI to use the compatibility utility instead of deprecated methods
- Updates KerberosAuthenticator to use the new utility for getting current subject
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| UserGroupInformation.java | Updated to use SubjectUtil for getting current subject and executing privileged actions |
| SubjectUtil.java | New utility class providing compatibility between deprecated and new Subject APIs |
| KerberosAuthenticator.java | Updated to use SubjectUtil for getting current subject |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/SubjectUtil.java
Outdated
Show resolved
Hide resolved
…oop/util/SubjectUtil.java Co-authored-by: Copilot <[email protected]>
|
This PR is ready for review. cc @steveloughran @stoty @cnauroth @slfan1989 @jbrinegar Also cc @dongjoon-hyun @LuciferYang from the Spark community, this unblocks Spark to support JDK 25 |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
trivial checkstyles need fixing |
|
@steveloughran just fixed 1 and 4, but 2 and 3 look weird, why the static var name should start with a lower-case letter? |
steveloughran
requested changes
Aug 20, 2025
|
|
||
| @Private | ||
| public class SubjectUtil { | ||
| private static MethodHandle CALL_AS; |
There was a problem hiding this comment.
thes should be final, as the static sections are trying to set them up as such. This may need some tuning of that code so that the compiler is happy there's only one attempt ever made to set it.
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private static <E extends Throwable> RuntimeException sneakyThrow(Throwable e) throws E { |
There was a problem hiding this comment.
add javadoc -it is a use of templates I've not seen before...having the type of raised exception inferred from the context around the invocation
There was a problem hiding this comment.
this article explains how it works
https://www.baeldung.com/java-sneaky-throws
| * thrown by {@code action.call()}. | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public static <T> T callAs(Subject subject, Callable<T> action) throws CompletionException { |
There was a problem hiding this comment.
add test, verify that
- callable throwing a PrivilegedActionException is mapped to completion exception
- callable raising any RTE is handled correctly
| * @return the result of the action | ||
| * @param <T> the type of the result | ||
| */ | ||
| public static <T> T doAs(Subject subject, PrivilegedAction<T> action) { |
There was a problem hiding this comment.
tests to verify handling. what happens if the callable raises a CompletionException without an inner cause? that "should never happen" path is reached -so verify
There was a problem hiding this comment.
I see, it's possible in the legacy doAs case
There was a problem hiding this comment.
during constructing the test cases, I realized the current approach seems impossible to keep original exception propagation behavior ...
for example, in JDK 17, SubjectUtil.callAs binds to Subject.doAs, it's unable to distinguish 1) action throws a PrivilegedActionException, from 2) the bound doAs wrap the original exception thrown by action with a PrivilegedActionException
There was a problem hiding this comment.
the exception propagation mechanism is changed in Java 12 ... I refactored the code, and added TestSubjectUtil to cover all cases I can imagine.
Now, the UT passed under JDK 8, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25
mvn -Dsurefire.failIfNoSpecifiedTests=false install -pl :hadoop-auth -am -Dtest=TestSubjectUtil
There was a problem hiding this comment.
So doAs() throws NPE and Security exceptions directly, and wraps Exceptions from the Action in a PrivilegedExceptionAction , while
callAs() wraps every Exception in a CompletionException.
So yes, we do lose some information when using callAs(), as we can't (easily) distinguish between NPEs and SecurityExceptions thrown in doAs itself or in the callable.
However, UGI.doAs() unwraps PrivilegedActionExceptions anyway (and adds the IOException constraint) so I can't really see any case when this would actually matter.
So by shortcutting doAs() for older JDKs the only difference is that we're throwing NPEs and SecurityExceptions from the doAs() method itself directly, while in the wrapped case we're wrapping them in an UndeclaredThrowableException, and even this is only matters in JDK 17, and JDK18+ wraps all exceptions in a CompletionException anyway (just like SubjectUtil does)
I support adding the shortcut, but more as optimization than a correctness fix.
There was a problem hiding this comment.
I see, I added Objects.requireNonNull(action) at the begin of each method
| if (cause != null) { | ||
| throw sneakyThrow(cause); | ||
| } else { | ||
| // This should never happen, as CompletionException should always wrap an exception |
There was a problem hiding this comment.
unless the invoked callable raises it
| import org.apache.hadoop.classification.InterfaceAudience.Private; | ||
|
|
||
| @Private | ||
| public class SubjectUtil { |
There was a problem hiding this comment.
add javadocs.
I think there should also be some boolean indicating whether the new or old methods were picked up. This can be accompanied by a unit test which verifies that on java > 17 the new ones are found.
|
@steveloughran thanks for your feedback, will address ASAP. BTW, I see that Hadoop 3.4.2 RC2 failed, if this PR gets merged before 3.4.2 is released, would you mind including it in 3.4.2? (I don't mean block the release, just nice to have) |
LuciferYang
reviewed
Aug 20, 2025
| SubjectUtil.class, "callableToPrivilegedExceptionAction", convertSignature); | ||
| CALL_AS = MethodHandles.filterArguments(doAs, 1, converter); | ||
| } catch (NoSuchMethodException e) { | ||
| throw new AssertionError(e); |
There was a problem hiding this comment.
I suggest throwing ExceptionInInitializerError rather than AssertionError in the static block.
There was a problem hiding this comment.
thanks for suggestion, adopted
|
|
||
| @SuppressWarnings("unchecked") | ||
| private static <E extends Throwable> RuntimeException sneakyThrow(Throwable e) throws E { | ||
| throw (E) e; |
There was a problem hiding this comment.
Type checking should be performed here. For RuntimeException and Error, they should be thrown directly, while other exceptions should be wrapped into a RuntimeException and then thrown.
Additionally, does this method need to specify a return value type?
There was a problem hiding this comment.
the intention here is to allow the caller method to throw the checked exception without explicitly exception list declaration
https://www.baeldung.com/java-sneaky-throws
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@pan3793 Thank you for your contribution! From my perspective, I would give this a +0.5, mainly because I’m not very familiar with this part. However, I hope this PR can receive consensus and full approval from the other reviewers. cc: @stoty @steveloughran @cnauroth @cxzl25 @LuciferYang @dongjoon-hyun |
|
@ahmarsuhail Pan asked me offline whether this feature could be released in 3.4.2. Do you think that’s possible? I see that the vote for 3.4.2 is still ongoing — can we include this PR in RC3? |
slfan1989
approved these changes
Aug 23, 2025
LuciferYang
approved these changes
Aug 25, 2025
|
It would be very delightful if this pr could be included in 3.4.2. |
stoty
reviewed
Aug 25, 2025
...ct/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/SubjectUtil.java
Show resolved
Hide resolved
stoty
approved these changes
Aug 25, 2025
@stoty Thank you very much for reviewing the code! |
cxzl25
approved these changes
Aug 25, 2025
|
🎊 +1 overall
This message was automatically generated. |
|
@stoty @cxzl25 @LuciferYang Thank you all for your help and support! @pan3793 Thanks for the contribution! I will add @steveloughran @stoty @cxzl25 @LuciferYang to the review list. I will add @stoty as a co-author. |
dongjoon-hyun
approved these changes
Aug 25, 2025
|
@pan3793 @stoty Thanks for the contribution! @steveloughran @cxzl25 @LuciferYang @cnauroth Thanks for the review! |
pan3793
added a commit
to pan3793/hadoop
that referenced
this pull request
Aug 26, 2025
* Hadoop UGI compatible with Java 25 Co-authored-by: Istvan Toth <[email protected]> Reviewed-by: Shaoyun Chen <[email protected]> Reviewed-by: Dongjoon Hyun <[email protected]> Reviewed-by: Yang Jie <[email protected]> Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: Chris Nauroth <[email protected]> Reviewed-by: Istvan Toth <[email protected]> Signed-off-by: Shilun Fan <[email protected]>
|
@slfan1989 @pan3793 Voting for RC3 is currently on-going, started last last thursday. We have 5 binding votes already so I think we should be able to successfully release 3.4.2 soon. If for any reason that doesn't happen, we can get this into 3.4.2 |
|
@ahmarsuhail thank you for the information |
slfan1989
pushed a commit
that referenced
this pull request
Aug 27, 2025
* HADOOP-19212. Hadoop UGI compatible with Java 25. Co-authored-by: Istvan Toth <[email protected]> Signed-off-by: Shilun Fan <[email protected]>
@ahmarsuhail Sorry for the late reply. I’ve discussed with @pan3793 offline, and we plan to release this PR in the next version, as additional PRs may be needed to ensure greater stability. Thank you all for your contributions to the 3.4.2 release！ |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Co-authored-by: Justin Brinegar [email protected]
Co-authored-by: Istvan Toth [email protected]
Description of PR
https://docs.oracle.com/en/java/javase/24/security/migrating-deprecated-removal-subject-getsubject-and-subject-doas-methods-subject-current-and-subje.html
This PR is extracted from @stoty's PR #7434, with some tweaks, which is an alternative to #7081
The main goal is to make minimal changes to make the Hadoop client compatible with Java 25, which unlocks downstream projects that rely on the Hadoop client, e.g. Spark, to support Java 25.
How was this patch tested?
Spark Integration
Integrated with Spark 4.1.0-SNAPSHOT (master branch) with Java 25.
Steps
Install JDK 25-ea and set up
JAVA_HOMEenvPull this PR, then compile and install Hadoop jars to maven local repo.
Pull Spark master code, and
Modify
pom.xmlto use the compiled Hadoop JarsRun UT with the compiled Hadoop Jars
Before
After
With this patch, Spark passes all UTs of the YARN module with Java 25, a few Hive SQL tests fail with irrelevant issues.
Run Spark Job on Kerberized YARN/HDFS cluster
@cxzl25 also helps test this patch using a Spark job on a Kerberized cluster with Java 25, basic SQL, including reading and writing Hive tables, which works as expected.
Hadoop Existing UT
I set up a GitHub Actions workflow in my forked repo to run Hadoop UT with Java 17 and 25.
Note: I have to exclude some UTs because they are flaky or failed consistently even without this PR.
The test jobs show a positive report of this change.
https://github.com/pan3793/hadoop/actions/runs/17120184276
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?