HADOOP-19298. [JDK17] Add a JDK17 profile. #7085
Merged
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
|
💔 -1 overall
This message was automatically generated. |
|
@adoroszlai I see that Ozone already supports JDK 17. Do you have any other suggestions for adding the profile? |
|
@steveloughran Could you help review this PR? Thank you very much! Do you have any feedback or suggestions regarding the addition of the profile? |
zeekling
approved these changes
Oct 1, 2024
steveloughran
approved these changes
Oct 1, 2024
@steveloughran Thank you for your message! I agree with your point, and we will proceed to merge this PR first. |
pan3793
reviewed
Oct 2, 2024
hadoop-project/pom.xml
Outdated
| <properties> | ||
| <maven.compiler.release>17</maven.compiler.release> | ||
| <argLine> | ||
| --add-opens java.base/jdk.internal.ref=ALL-UNNAMED |
There was a problem hiding this comment.
Thank you for your question! This list comes from the additional compilation items required by our internal Hadoop version to support JDK 17. Currently, the list might contain some redundancy as the community is continuously working on JDK 17 improvements, and our internal version has not merged all the latest PRs from the community.
There was a problem hiding this comment.
How does this interact with existing setting of argLine?
Lines 2459 to 2463 in 3f637ef
Lines 170 to 177 in 3f637ef
Does it simply override the value coming from maven-surefire-plugin.argLine? If so, can we tweak that property instead, to avoid confusion? With -XX:+IgnoreUnrecognizedVMOptions we can do that regardless of Java version (i.e. outside of this profile).
There was a problem hiding this comment.
@slfan1989 @adoroszlai I made a more accurate JPMS option list at #7114 by running all UTs with JDK 17, and added them to global maven-surefire-plugin.argLine with the option -XX:+IgnoreUnrecognizedVMOptions
There was a problem hiding this comment.
<profile>
<id>java9</id>
<activation>
<jdk>[9,)</jdk>
</activation>
<properties>
<maven.compiler.release>${javac.version}</maven.compiler.release>
</properties>
</profile>
The Javadoc configuration can be removed, as it currently doesn’t cause any extra errors since Pan has submitted a PR to fix the Javadoc content.
cc: @pan3793 @adoroszlai
There was a problem hiding this comment.
My suggestion is just
+ <profile>
+ <id>java9</id>
+ <activation>
+ <jdk>[9,)</jdk>
+ </activation>
+ <properties>
+ <maven.compiler.release>${javac.version}</maven.compiler.release>
+ </properties>
+ </profile>and it's encouraged to
use version numbers without the "1." prefix (supported since javac 5)
- <javac.version>1.8</javac.version>
+ <javac.version>8</javac.version>There was a problem hiding this comment.
I readed the document provided by Pan, and I find it reasonable.
- <javac.version>1.8</javac.version>
+ <javac.version>8</javac.version>
cc: @steveloughran
|
I tested the following building command without this patch, also works well. OS, JDK, Maven information |
I need to confirm this issue and then provide a response. Based on the process of upgrading the JDK version in other components, we will likely need some additional build options. |
adoroszlai
reviewed
Oct 2, 2024
hadoop-project/pom.xml
Outdated
| <properties> | ||
| <maven.compiler.release>17</maven.compiler.release> | ||
| <argLine> | ||
| --add-opens java.base/jdk.internal.ref=ALL-UNNAMED |
There was a problem hiding this comment.
How does this interact with existing setting of argLine?
Lines 2459 to 2463 in 3f637ef
Lines 170 to 177 in 3f637ef
Does it simply override the value coming from maven-surefire-plugin.argLine? If so, can we tweak that property instead, to avoid confusion? With -XX:+IgnoreUnrecognizedVMOptions we can do that regardless of Java version (i.e. outside of this profile).
hadoop-project/pom.xml
Outdated
Comment on lines
2741
to
2743
| <id>jdk17</id> | ||
| <activation> | ||
| <jdk>[17,)</jdk> |
There was a problem hiding this comment.
Probably worth activating for Java 9+, where maven.compiler.release is supported.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
myandpr
reviewed
Oct 7, 2024
hadoop-project/pom.xml
Outdated
| --add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED | ||
| --add-exports java.base/sun.nio.ch=ALL-UNNAMED | ||
| --add-exports java.security.jgss/sun.security.krb5=ALL-UNNAMED | ||
| --add-exports java.base/sun.security.x509=ALL-UNNAMED |
There was a problem hiding this comment.
@slfan1989 Why are some formats “--add-exports=XX” while others are "--add-exports XX"? Perhaps you could standardize the format. Other than, it works well.
Additionally, i'm very interested in Hadoop supporting JDK 17, and I'm willing to take on related work.
There was a problem hiding this comment.
@myandpr Thank for the review! I will continue to improve this PR. If there are any PRs that can contribute to JDK 17, I can also help review them. We need to complete support for JDK 17 as soon as possible.
|
Hadoop on jdk17 can support namenode with more java heap size . And namenode will support more inodes. That is great！ |
|
💔 -1 overall
This message was automatically generated. |
slfan1989
force-pushed
the
HADOOP-19298
branch
from
f6d0921 to
7ffc194
Compare
|
@adoroszlai @pan3793 In #7114, Pan helped organize the additional parameters needed for JDK 17 and added them to the compilation command, allowing us to simplify our profile. The related JavaDoc issues were fixed in #6976, so we no longer need to add the extra options for JavaDoc. Could you please help review this change again? Thank you very much! |
myandpr
approved these changes
Oct 17, 2024
|
💔 -1 overall
This message was automatically generated. |
pan3793
approved these changes
Oct 17, 2024
adoroszlai
approved these changes
Oct 17, 2024
|
@steveloughran @zeekling @pan3793 @adoroszlai @myandpr @LiuGuH Thanks for the review! |
|
by any chance is this breaking the JDK-11 builds? |
|
@ayushtkn thanks for checking
diff --git hadoop-project/pom.xml hadoop-project/pom.xml
index 4e0b7475cc5..d35e3d399a0 100644
--- hadoop-project/pom.xml
+++ hadoop-project/pom.xml
@@ -157,7 +157,7 @@
<os-maven-plugin.version>1.7.0</os-maven-plugin.version>
<!-- define the Java language version used by the compiler -->
- <javac.version>1.8</javac.version>
+ <javac.version>8</javac.version>
<!-- The java version enforced by the maven enforcer -->
<!-- more complex patterns can be used here, such asCreated HADOOP-19318. |
slfan1989
added a commit
that referenced
this pull request
Oct 28, 2024
…d by Shi…" This reverts commit f931ede.
|
@ayushtkn @adoroszlai Thank you for discussing this issue. I have reverted this PR and will wait for the replacement of sun.misc before proceeding with the upgrade. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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: HADOOP-19298. [JDK17] Add a JDK17 profile.
Using this profile, we can compile a JDK 17 version of Hadoop locally (without native).
I will also try to compile on other build platforms.
How was this patch tested?
For code changes:
Add a JDK 17 profile.