[fix][functions] Fix netty.DnsResolverUtil compat issue on JDK9+ for the function Runtimes #16423
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
eolivelli
reviewed
Jul 6, 2022
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
Outdated
Show resolved
Hide resolved
dlg99
reviewed
Jul 6, 2022
| @@ -320,6 +320,11 @@ public static List<String> getCmd(InstanceConfig instanceConfig, | |||
|
|
|||
| args.add("-Dio.netty.tryReflectionSetAccessible=true"); | |||
|
|
|||
| // Needed for netty.DnsResolverUtil on JDK9+ | |||
| if (!System.getProperty("java.version").startsWith("1.")) { | |||
| args.add("--add-opens java.base/sun.net=ALL-UNNAMED"); | |||
There was a problem hiding this comment.
Do you want to add other modules too? at least look at the changes that resulted in these and see if they are applicable to the functions
$ ggrep -rI "\-\-add-opens"
buildtools/pom.xml: --add-opens java.base/jdk.internal.loader=ALL-UNNAMED
buildtools/pom.xml: --add-opens java.base/java.lang=ALL-UNNAMED <!--Mockito-->
buildtools/dependency-reduced-pom.xml: <test.additional.args>--add-opens java.base/jdk.internal.loader=ALL-UNNAMED
buildtools/dependency-reduced-pom.xml: --add-opens java.base/java.lang=ALL-UNNAMED</test.additional.args>
bin/bookkeeper:# Start --add-opens options
bin/bookkeeper:# '--add-opens' option is not supported in jdk8
bin/bookkeeper: OPTS="$OPTS --add-opens java.base/java.io=ALL-UNNAMED --add-opens java.base/java.util.zip=ALL-UNNAMED"
bin/bookkeeper: OPTS="$OPTS --add-opens java.base/java.nio=ALL-UNNAMED --add-opens java.base/jdk.internal.misc=ALL-UNNAMED"
bin/function-localrunner:# Start --add-opens options
bin/function-localrunner:# '--add-opens' option is not supported in jdk8
bin/function-localrunner: OPTS="$OPTS --add-opens java.base/java.nio=ALL-UNNAMED --add-opens java.base/jdk.internal.misc=ALL-UNNAMED"
bin/pulsar:# Start --add-opens options
bin/pulsar:# '--add-opens' option is not supported in jdk8
bin/pulsar: OPTS="$OPTS --add-opens java.base/java.io=ALL-UNNAMED --add-opens java.base/java.util.zip=ALL-UNNAMED"
bin/pulsar: OPTS="$OPTS --add-opens java.base/java.nio=ALL-UNNAMED --add-opens java.base/jdk.internal.misc=ALL-UNNAMED"
bin/pulsar: OPTS="$OPTS --add-opens java.base/sun.net=ALL-UNNAMED --add-opens java.management/sun.management=ALL-UNNAMED"
bin/pulsar-client:# Start --add-opens options
bin/pulsar-client:# '--add-opens' option is not supported in jdk8
bin/pulsar-client: OPTS="$OPTS --add-opens java.base/sun.net=ALL-UNNAMED"
pom.xml: --add-opens java.base/jdk.internal.loader=ALL-UNNAMED
pom.xml: --add-opens java.base/java.lang=ALL-UNNAMED <!--Mockito-->
pom.xml: --add-opens java.base/java.io=ALL-UNNAMED <!--Bookkeeper NativeIO -->
pom.xml: --add-opens java.base/sun.net=ALL-UNNAMED <!--netty.DnsResolverUtil-->
pom.xml: --add-opens java.management/sun.management=ALL-UNNAMED <!--JvmDefaultGCMetricsLogger-->
There was a problem hiding this comment.
java.base/sun.netshould be enough for Pulsar client since it's the same configuration used in pulsar-client CLI tool.
nicoloboschi
requested changes
Jul 7, 2022
| @@ -320,6 +320,11 @@ public static List<String> getCmd(InstanceConfig instanceConfig, | |||
|
|
|||
| args.add("-Dio.netty.tryReflectionSetAccessible=true"); | |||
|
|
|||
| // Needed for netty.DnsResolverUtil on JDK9+ | |||
| if (!System.getProperty("java.version").startsWith("1.")) { | |||
There was a problem hiding this comment.
what about using commons-lang3 utility
SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9) ?
There was a problem hiding this comment.
we should have a consistent approach across the code base, do you see other cases in Java code where we detect the Java version ?
There was a problem hiding this comment.
No. That's the only place I found
cbornet
force-pushed
the
functions-jdk11
branch
2 times, most recently
from
095032d
to
23ab6be
Compare
nicoloboschi
reviewed
Jul 7, 2022
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeUtils.java
Outdated
Show resolved
Hide resolved
cbornet
force-pushed
the
functions-jdk11
branch
2 times, most recently
from
3e43cf8
to
0c306ca
Compare
nicoloboschi
approved these changes
Jul 7, 2022
|
/pulsarbot rerun-failure-checks |
2 similar comments
|
/pulsarbot rerun-failure-checks |
|
/pulsarbot rerun-failure-checks |
|
/pulsarbot rerun-failure-checks |
cbornet
force-pushed
the
functions-jdk11
branch
2 times, most recently
from
e1691be
to
9cace6d
Compare
…the function Runtimes
|
/pulsarbot rerun-failure-checks |
cbornet
added a commit
to cbornet/pulsar
that referenced
this pull request
Aug 31, 2022
…the function Runtimes (apache#16423)
nicoloboschi
pushed a commit
to datastax/pulsar
that referenced
this pull request
Sep 1, 2022
…the function Runtimes (apache#16423)
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.
Motivation
DnsResolverUtil fails to get existing DNS TTL settings. This happens in Java 17.
This has been fixed for the pulsar-client and pulsar launcher scripts in #15349 and #15540
This PR is to do the same for the Runtime java command.
Modifications
Detect the java version of the function worker and add the JVM flag
--add-opens java.base/sun.net=ALL-UNNAMEDif the version is 9+Documentation
Check the box below or label this PR directly.
Need to update docs?
doc-required(Your PR needs to update docs and you will update later)
doc-not-neededfix
doc(Your PR contains doc changes)
doc-complete(Docs have been already added)