YARN-11796. Accept extra JDK 17 options in tests #7512
Merged
Conversation
This file contains 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
|
🎊 +1 overall
This message was automatically generated. |
|
@pan3793 Could you review this PR? Thank you very much! I remember you made similar changes, but I'm not sure if the changes in this PR are necessary. |
pan3793
reviewed
Mar 21, 2025
|
|
||
| if (Shell.isJavaVersionAtLeast(17)) { | ||
| assertTrue(result.remove("--add-exports=java.base/sun.net.dns=ALL-UNNAMED")); | ||
| assertTrue(result.remove("--add-exports=java.base/sun.net.util=ALL-UNNAMED")); |
There was a problem hiding this comment.
MAPREDUCE-7456 defines
private static final String ADDITIONAL_JDK17_PLUS_OPTIONS =
"--add-opens=java.base/java.lang=ALL-UNNAMED " +
"--add-exports=java.base/sun.net.dns=ALL-UNNAMED " +
"--add-exports=java.base/sun.net.util=ALL-UNNAMED";
Why should only the last two be removed?
BTW, could you please leave a few comments that at least mention MAPREDUCE-7456 ?
There was a problem hiding this comment.
Thank you @pan3793 .
JDK17 options are defined at several locations.
Other than the one you mentioned, they are also defined in org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.ContainerLocalizer which only adds
these two, and apparently these tests trigger that one.
If I added all three, the tests would fail, as --add-opens=java.base/java.lang=ALL-UNNAMED is never added for those test cases.
This also bothers me, I have opened HADOOP-19505 earlier to track this. (Though I have no immediate plans to work on that)
There was a problem hiding this comment.
As for comments, most tests in TestContainerLocalizer already have similar ifs for JDK17 (I guess these were just added later, and never tested with JDK17).
Would you like me to add comments to those as well ?
There was a problem hiding this comment.
Would you like me to add comments to those as well ?
Yes, please do that, I think the brief comments should help future explorers easy to understand the test code for this case
pan3793
approved these changes
Mar 21, 2025
|
🎊 +1 overall
This message was automatically generated. |
|
@slfan1989 Could you take another look, and commit if you are satisfied ? |
slfan1989
approved these changes
Mar 26, 2025
@stoty Thanks for the contribution! LGTM. |
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.
Description of PR
JIRA: YARN-11796. Accept extra JDK 17 options in tests
Accept options automatically added when run on JDK17+ in relevant tests
How was this patch tested?
Ran the modified tests with JDK8 and JDK17
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?