HADOOP-19219. Resolve Certificate error in Hadoop-auth tests. #6939
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
is it only required for test but not required at runtime? |
@pan3793 I have been trying to compile it using maven locally and during compilation I have faced this issue. |
|
of course, those parameters are not required for compilation, but it's dangerous to only add them for test but not runtime. |
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <configuration> | ||
| <argLine>--add-exports=java.base/sun.security.x509=ALL-UNNAMED --add-exports=java.base/sun.security.util=ALL-UNNAMED</argLine> |
There was a problem hiding this comment.
BTW, I would suggest adding those parameters unconditionally, just like Apache Spark does.
There was a problem hiding this comment.
@pan3793 Can you please clarify "unconditionally" here? Do you mean "regardless of Java version"? --add-exports and --add-opens do not work with Java 8. I guess Spark does not support building/testing with Java 8.
There was a problem hiding this comment.
Can you please clarify "unconditionally" here? Do you mean "regardless of Java version"?
correct.
I guess Spark does not support building/testing with Java 8
Spark 3.3 ~ 3.5 supports Java 8 ~ 17, you can check Spark branch-3.5
https://github.com/apache/spark/blob/branch-3.5/pom.xml#L300
There was a problem hiding this comment.
Thanks. Spark build makes it work by also setting -XX:+IgnoreUnrecognizedVMOptions:
https://github.com/apache/spark/blob/f1f5bb6ee40b655e334e90c0f81042c632601d41/pom.xml#L300-L302
There was a problem hiding this comment.
yes, sorry, i should mention IgnoreUnrecognizedVMOptions too.
do you think that hadoop can also follow that? it would simplify shell scripts and pom.xml
There was a problem hiding this comment.
I'm happy with -XX:+IgnoreUnrecognizedVMOptions in our build/test scripts
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <configuration> | ||
| <source>${maven.compiler.source}</source> | ||
| <target>${maven.compiler.target}</target> | ||
| </configuration> | ||
| </plugin> |
There was a problem hiding this comment.
Setting properties maven.compiler.source and maven.compiler.target has the same effect as creating a configuration for the plugin with source and target set. We only need one of these changes.
I'd keep the properties.
| <plugin> | |
| <groupId>org.apache.maven.plugins</groupId> | |
| <artifactId>maven-compiler-plugin</artifactId> | |
| <configuration> | |
| <source>${maven.compiler.source}</source> | |
| <target>${maven.compiler.target}</target> | |
| </configuration> | |
| </plugin> |
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-enforcer-plugin</artifactId> | ||
| <version>3.0.0</version> |
There was a problem hiding this comment.
nit: version is not needed, defined in root POM.
| <version>3.0.0</version> |
| <executions> | ||
| <execution> | ||
| <id>enforce-java-version</id> | ||
| <goals> | ||
| <goal>enforce</goal> | ||
| </goals> | ||
| <configuration> | ||
| <rules> | ||
| <requireJavaVersion> | ||
| <version>1.8</version> | ||
| </requireJavaVersion> | ||
| </rules> | ||
| </configuration> | ||
| </execution> | ||
| </executions> |
There was a problem hiding this comment.
Why require Java version 1.8 in a profile activated by <jdk>1.8</jdk>?
| <maven.compiler.source>${javac.version}</maven.compiler.source> | ||
| <maven.compiler.target>${javac.version}</maven.compiler.target> |
There was a problem hiding this comment.
Looks like these properties are set regardless of Java version. So they could be moved out of the profiles.
However, we can use the profiles to set maven.compiler.release for Java 9+ instead of the other two properties.
Java 9 introduced new javac option --release, which helps ensure compatibility with the given Java version (blog with great explanation). The goal of this change is to set this option when compiling with JDK 9+, instead of --source and --target.
--release expects version without the old-style 1. prefix (i.e. 8 instead of 1.8). maven-enforcer-plugin added support for the same in version 3.2.1, so we also need to bump the plugin.
There was a problem hiding this comment.
do you want to leverage the --release to allow Hadoop building against Java 17 but could run with Java 8 and above?
There was a problem hiding this comment.
source and target are set for the same reason, to be able to build with newer Java, but still create binaries for Java 8. release is similar, but better.
There was a problem hiding this comment.
I'm not sure if it really works. I have experienced a lot of the following error cases when running jar on JDK 8 that built with newer JDK with release 8
java.lang.NoSuchMethodError: java.nio.ByteBuffer.limit(I)Ljava/nio/ByteBuffer
There was a problem hiding this comment.
I have experienced a lot of the following error cases when running jar on JDK 8 that built with newer JDK with release 8
Using -target or --release?
Because that's exactly the kind of problem --release solves compared to -target.
It's easy to reproduce the problem and verify the fix:
$ cat > NoSuchMethod.java <<EOF
import java.nio.ByteBuffer;
public class NoSuchMethod {
public static void main(String[] args) {
System.out.println(ByteBuffer.allocate(100).position(20).limit(62).remaining());
}
}
EOF
$ usr/lib/jvm/java-11-openjdk-amd64/bin/javac -Xlint:none -source 8 -target 8 NoSuchMethod.java \
&& /usr/lib/jvm/java-8-openjdk-amd64/bin/java NoSuchMethod
Exception in thread "main" java.lang.NoSuchMethodError: java.nio.ByteBuffer.position(I)Ljava/nio/ByteBuffer;
at NoSuchMethod.main(NoSuchMethod.java:6)
$ /usr/lib/jvm/java-11-openjdk-amd64/bin/javac --release 8 NoSuchMethod.java \
&& /usr/lib/jvm/java-8-openjdk-amd64/bin/java NoSuchMethod
42There was a problem hiding this comment.
Wow, thanks for letting me know it can be resolved by --release
There was a problem hiding this comment.
that bytebuffer thing is trouble as even recent java8 open jdks have that bytebuffer override; you need to build on an oracle jdk right now. this will be an improvement if we can trust it
| <profile> | ||
| <id>java-8</id> | ||
| <activation> | ||
| <jdk>1.8</jdk> | ||
| </activation> |
There was a problem hiding this comment.
Why are these profiles being added in hadoop-common-project/hadoop-auth/pom.xml instead of the root POM?
- We may need the
--add-opensargs for other tests. - More importantly, the compiler settings should be global.
There was a problem hiding this comment.
Why are these profiles being added in
hadoop-common-project/hadoop-auth/pom.xmlinstead of the root POM?
- We may need the
--add-opensargs for other tests.- More importantly, the compiler settings should be global.
@adoroszlai Sure I will move it to hadoop-main pom.
|
FYI, |
|
@adoroszlai, @pan3793 Thank you for the review. I have changed implementation as per your suggestions. Please have a look! |
|
💔 -1 overall
This message was automatically generated. |
|
Runtime JPMS args mentioned here #6939 (comment) should be considered, otherwise the runtime errors will be hidden in testing. |
| @@ -167,9 +167,11 @@ | |||
| --> | |||
| <enforced.java.version>[${javac.version},)</enforced.java.version> | |||
| <enforced.maven.version>[3.3.0,)</enforced.maven.version> | |||
|
|
|||
| <extraJavaTestArgs> | |||
There was a problem hiding this comment.
jdk8 does not support --add-exports, and the test will fail.
|
💔 -1 overall
This message was automatically generated. |
| @@ -618,8 +618,7 @@ function hadoop_bootstrap | |||
|
|
|||
| export HADOOP_OS_TYPE=${HADOOP_OS_TYPE:-$(uname -s)} | |||
|
|
|||
| # defaults | |||
| export HADOOP_OPTS=${HADOOP_OPTS:-"-Djava.net.preferIPv4Stack=true"} | |||
| export HADOOP_OPTS="${HADOOP_OPTS:-"-Djava.net.preferIPv4Stack=true"} -XX:+IgnoreUnrecognizedVMOptions --add-exports=java.base/sun.security.x509=ALL-UNNAMED --add-exports=java.base/sun.security.util=ALL-UNNAMED" | |||
There was a problem hiding this comment.
I would suggest adding the JPMS opts in function hadoop_finalize_hadoop_opts or add a dedicated function hadoop_finalize_jpms_opts
also, we should add comments on both sides(here and pom.xml), to mention the future contributors sync the jpms opts
There was a problem hiding this comment.
I concur. we also need to make sure all forked processes (terasort...) are happy
Description of PR
Implemented JDK 17 profile to suppress sun.security certificate errors encountered during migration from JDK 8, ensuring seamless compilation and compatibility.
How was this patch tested?
Tested locally by compiling project on JDK17.
For code changes:
Added profile for JDK17.