ARROW-15755: [Java] Support Java 17 #12941

davisusanibar · 2022-04-20T22:19:26Z

Changes needed to support JDK17:

There are some errorprone suppress warnings needed to be able to continue using JSE17 with JSE1.8 source/target. This need to be migrated to new JSE X base on ML discussion.
JSE11+ is modular for that reason was needed to add --add-exports and --add-opens
The --add-exports option allows code in the target module to access types in the named package of the source module if the target module reads the source module.
If you have to allow code on the class path to do deep reflection to access nonpublic members, then use the --add-opens runtime option.
To be able to running unit test was needed to configure maven-surefire-plugin with <argLine>--add-opens=java.base/java.nio=ALL-UNNAMED</argLine>

github-actions · 2022-04-20T22:19:46Z

https://issues.apache.org/jira/browse/ARROW-15755

…g.apache.hadoop.shaded.com.ctc.wstx.io.InputBootstrapper

lidavidm · 2022-04-21T00:48:52Z

You will want to rebase to get CI to run.

Can we add a CI job that tests JDK17?

lidavidm

Thanks for working this out. I think we just need to make sure to cover this in CI.

lidavidm · 2022-04-21T00:51:10Z

java/pom.xml

@@ -736,6 +738,7 @@
           See https://github.com/jbosstools/m2e-apt/issues/62 for details
      -->
      <activation>
+        <jdk>1.8</jdk>


Hmm, we already have a 1.8-specific profile below. What's the difference?

lidavidm · 2022-04-21T00:54:24Z

java/pom.xml

+                <arg>-XDcompilePolicy=simple</arg>
+                <arg>
+                  -Xplugin:ErrorProne \
+                  -XepExcludedPaths:.*/(target/generated-sources)/.*


nit, but can these be two separate <arg> blocks or does it not work that way?

It does not work on that way.

Be aware that when running on JDK 8 the flags cannot be wrapped across multiple lines. JDK 9 and above do allow the flags to be separated by newlines. That is, the second element above can also be formatted as follows on JDK 9+, but not on JDK 8:

lidavidm · 2022-04-21T00:56:01Z

java/memory/memory-core/src/test/java/org/apache/arrow/memory/util/TestArrowBufPointer.java

@@ -111,6 +111,7 @@ public void testNullPointersHashCode() {
  }

  @Test
+  @SuppressWarnings("ReturnValueIgnored")


Maybe instead of suppresing we can check that the hash code changes/doesn't change?

We can remove the suppression now right?

lidavidm · 2022-04-21T00:56:23Z

java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java

@@ -77,6 +77,7 @@
 /**
 * Flight client with Flight SQL semantics.
 */
+@SuppressWarnings({"ProtoBuilderReturnValueIgnored", "ReturnValueIgnored"})


Hmm, this is a very wide scope to suppress. Is it possible to refactor things?

lidavidm · 2022-04-21T00:57:13Z

java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestCallOptions.java

@@ -97,6 +97,7 @@ public void mixedProperties() {
    testHeaders(headers);
  }

+  @SuppressWarnings("ReturnValueIgnored")


Same here, maybe an assertTrue(....hasNext())?

lidavidm · 2022-04-21T00:57:32Z

java/adapter/orc/pom.xml

+                    <groupId>commons-logging</groupId>
+                    <artifactId>commons-logging</artifactId>


Can we add a brief comment about why we need to exclude these?

Yes, the reason is [WARNING] Rule 0: org.apache.maven.plugins.enforcer.BannedDependencies failed with message: Found Banned Dependency: commons-logging:commons-logging:jar:1.1.3. Let me add some message on the exclude tag.

Ah, I didn't realize we had a bannedDependencies set. Ok, it's probably fine to leave this as is then.

Though uh wow, that's been there since the initial Arrow commit in 2016. Are we sure Hadoop functions without this dependency, incidentally?

Unit test is working without problems in JSE 8, JSE 11 but not in JSE 17, that is the reason because we added now.

pitrou · 2022-04-21T09:10:44Z

@lwhite1 You might want to take a look at this.

lwhite1 · 2022-04-21T13:23:57Z

@pitrou will do

davisusanibar · 2022-04-21T14:14:10Z

You will want to rebase to get CI to run.

Can we add a CI job that tests JDK17?

For CI jobs please take a look at https://issues.apache.org/jira/browse/ARROW-15177. Thank for your support @raulcd

lidavidm · 2022-04-21T14:14:57Z

Well I don't think we should merge this without a CI job to actually validate it.

lidavidm · 2022-04-21T14:15:20Z

Either that, or we should make sure to validate JDK17 during the 8.0.0 vote.

lwhite1

Other than my one nit on SuppressWarnings, this looks good to me.

lwhite1 · 2022-04-21T14:16:06Z

java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java

@@ -246,9 +246,11 @@ public FlightClientMiddleware onCallStarted(CallInfo info) {
  }

  // Used to test that middleware can send and receive multi-valued text and binary headers.
+  @SuppressWarnings({"DoubleBraceInitialization"})


Maybe just initialize these values (here and below) the old-fashioned way, rather than add the SuppressWarnings?

Thanks, changed

lidavidm · 2022-04-21T18:41:12Z

java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java

+    EXPECTED_BINARY_HEADERS.put("x-binary-bin", Arrays.asList(new byte[] {0}, new byte[]{1}));
+    EXPECTED_TEXT_HEADERS.put("x-text", Arrays.asList("foo", "bar"));


Hmm, inline the declarations here too if they're only used in this test? Else put the initialization in a static block

java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java

hashhar · 2022-04-22T13:31:46Z

java/pom.xml

+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
+                <arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
+                <arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>


Does this change mean that consumers of this library will still need to pass add-opens to get Arrow to work with JDK 17? (I think so since you are just making the compilation work with these arguments, no code is changed to not use those modules?).

There are two part on this question:

Changed for arrow side: Changes related to --add-exports" are needed to continue supporting erroProne base on JSE11+ installation doc. It mean you won't need this changes if you run arrow java building code without errorProne validation (mvn clean install -P-error-prone-jdk11+ ....)

Changes as a user of arrow: If the user are planning to use Arrow with JSE17 is needed to pass modules needed. For example if I run cookbook for IO https://arrow.apache.org/cookbook/java/io.html it finished with an error mention Unable to make field long java.nio.Buffer.address accessible: module java.base does not "opens java.nio" to unnamed module for that reason as a user for JSE17 (not for arrow changes) is needed to add VM arguments as -ea --add-opens=java.base/java.nio=ALL-UNNAMED and it will finished without errors.

Please let me know if you have some additional doubts.

Can we add point (2) to the docs?

Can we add point (2) to the docs?

Hi Team, just send this PR to update Arrow JavaSE17/18 documentation. Please if you could review that, thanks.

@davisusanibar It looks like this is a half-finished solution because VM arguments should be considered a temporary solution. Is it possible to use ARROW with JDK 17 and not have VM arguments? Every time I see posts with VM arguments I feel a little worried. In JEP-261 there's a warning about the usage of --add-opens:

The --add-exports and --add-opens options must be used with great care. You can use them to gain access to an internal API of a library module, or even of the JDK itself, but you do so at your own risk: If that internal API is changed or removed then your library or application will fail.

Hi @iamzafar as per the jar artifacts are exposed as a legacy mode (it means any jar are running in the classpath is consider as a unnamed module) and base on JEP 200 The Modular JDK: yes, this is needed:

--add-opens ${module}/${package}=${target_modules} Let the ${target_modules} access all types and members, regardless of visibility, from ${module}'s ${package}

Yes, this is an intermediate step: (1) Legacy mode (already implemented, (2) single module and (3) multi-module will be the last steps.

This is a draft for implement JPMS and be more restrict about to not give ALL-UNNAMED access but there is a lot of discussion we need to close before to convert this draft into PR: #13072. This is the document that I am going to send for discussion to the ML.

Related to #12941 (comment) Closes #12990 from davisusanibar/java-ARROW-16292 Lead-authored-by: david dali susanibar arce <[email protected]> Co-authored-by: David Li <[email protected]> Signed-off-by: David Li <[email protected]>

raulcd · 2022-04-28T09:28:22Z

Hi @davisusanibar I have created this draft PR to add Java 17 to CI (#13021). As you can see from the triggered jobs, there is a new Java job:

It is obviously failing (https://github.com/apache/arrow/runs/6208174355?check_suite_focus=true) because the fixes for Java 17 are on your PR but I have cherry-picked the commit locally on top of your branch and I have validated the build is working and tests are passing. Tested locally with:

$ JDK=17 MAVEN=3.8.5 JDK_NAME=openjdk docker-compose run --rm debian-java

I have validated java version was the correct when testing:

bash-4.4# java --version
openjdk 17.0.2 2022-01-18
OpenJDK Runtime Environment (build 17.0.2+8-86)
OpenJDK 64-Bit Server VM (build 17.0.2+8-86, mixed mode, sharing)
bash-4.4# exit

I am not sure how we do this but probably you might want to cherrypick my commit and apply it on your PR.

lidavidm · 2022-04-28T12:30:13Z

Thanks @raulcd! I think we can merge this, then rebase your PR and check that it passes in CI, and then merge that as well?

java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java

lidavidm · 2022-04-28T12:58:04Z

java/memory/memory-core/src/test/java/org/apache/arrow/memory/util/TestArrowBufPointer.java

@@ -111,6 +111,7 @@ public void testNullPointersHashCode() {
  }

  @Test
+  @SuppressWarnings("ReturnValueIgnored")


We can remove the suppression now right?

java/pom.xml

raulcd · 2022-04-28T12:59:48Z

Thanks @raulcd! I think we can merge this, then rebase your PR and check that it passes in CI, and then merge that as well?

Thanks @lidavidm! that works for me, I'll be keeping an eye on this PR and will rebase mine once it is merged!

davisusanibar · 2022-04-28T14:23:10Z

Hi @davisusanibar I have created this draft PR to add Java 17 to CI (#13021). As you can see from the triggered jobs, there is a new Java job: It is obviously failing (https://github.com/apache/arrow/runs/6208174355?check_suite_focus=true) because the fixes for Java 17 are on your PR but I have cherry-picked the commit locally on top of your branch and I have validated the build is working and tests are passing. Tested locally with:
$ JDK=17 MAVEN=3.8.5 JDK_NAME=openjdk docker-compose run --rm debian-java
I have validated java version was the correct when testing:
bash-4.4# java --version
openjdk 17.0.2 2022-01-18
OpenJDK Runtime Environment (build 17.0.2+8-86)
OpenJDK 64-Bit Server VM (build 17.0.2+8-86, mixed mode, sharing)
bash-4.4# exit
I am not sure how we do this but probably you might want to cherrypick my commit and apply it on your PR.

Thanks @raulcd. Let me solve pending comments. Consider: in my local test was needed to add this environment variable "--add-opens=java.base/java.nio=ALL-UNNAMED", but as suggested let merge this and test your changes them.

…TestClientMiddleware.java Co-authored-by: David Li <[email protected]>

lidavidm · 2022-04-29T18:56:07Z

@github-actions crossbow submit java

github-actions · 2022-04-29T18:57:42Z

Revision: 439403b

Submitted crossbow builds: ursacomputing/crossbow @ actions-1988

Task	Status
java-jars
verify-rc-source-java-linux-almalinux-8-amd64
verify-rc-source-java-linux-conda-latest-amd64
verify-rc-source-java-linux-ubuntu-18.04-amd64
verify-rc-source-java-linux-ubuntu-20.04-amd64
verify-rc-source-java-linux-ubuntu-22.04-amd64
verify-rc-source-java-macos-amd64

toddfarmer · 2022-04-29T19:45:15Z

Just a note here as a reminder that https://arrow.apache.org/docs/dev/developers/java/building.html will need updates once support for additional JDK versions is added.

lidavidm · 2022-04-29T19:46:03Z

Good point. @davisusanibar can we include the docs changes here?

davisusanibar · 2022-04-29T20:04:26Z

Good point. @davisusanibar can we include the docs changes here?

Thanks @toddfarmer. Just updated.

docs/source/developers/java/building.rst

Co-authored-by: David Li <[email protected]>

pitrou · 2022-05-02T13:16:07Z

Great milestone, thanks a lot @davisusanibar !

This PR aims to add support for java 17 on CI as required on #12941 We probably should cherrypick the commit here on that PR. Closes #13021 from raulcd/ARROW-16327 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

ursabot · 2022-05-06T08:31:33Z

Benchmark runs are scheduled for baseline = 3f2c33f and contender = 760ad20. 760ad20 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.43% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.2% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 760ad200 ec2-t3-xlarge-us-east-2
[Finished] 760ad200 test-mac-arm
[Finished] 760ad200 ursa-i9-9960x
[Finished] 760ad200 ursa-thinkcentre-m75q
[Finished] 3f2c33f5 ec2-t3-xlarge-us-east-2
[Finished] 3f2c33f5 test-mac-arm
[Failed] 3f2c33f5 ursa-i9-9960x
[Finished] 3f2c33f5 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Support Java 17

acf79bd

github-actions bot added the Component: Java label Apr 20, 2022

Solving JDK17 adapter/orc error: java.lang.ClassNotFoundException: or…

865b520

…g.apache.hadoop.shaded.com.ctc.wstx.io.InputBootstrapper

davisusanibar changed the title ~~ARROW-15755: [Java] WIP - Support Java 17~~ ARROW-15755: [Java] Support Java 17 Apr 21, 2022

lidavidm self-requested a review April 21, 2022 00:48

Merge branch 'master' into java-ARROW-15755

7a050e8

lidavidm reviewed Apr 21, 2022

View reviewed changes

lwhite1 reviewed Apr 21, 2022

View reviewed changes

Solving PR comments

aea2f84

lidavidm reviewed Apr 21, 2022

View reviewed changes

Solving PR comments

cdfae57

hashhar reviewed Apr 22, 2022

View reviewed changes

davisusanibar mentioned this pull request Apr 25, 2022

ARROW-16292: [Java][Doc] Upgrade java documentation for JSE17/JSE18 #12990

Closed

raulcd mentioned this pull request Apr 28, 2022

ARROW-16327: [Java][CI] Add Java 17 to CI matrix for java workflows #13021

Closed

lidavidm reviewed Apr 28, 2022

View reviewed changes

davisusanibar and others added 3 commits April 28, 2022 09:28

Merge branch 'master' into java-ARROW-15755

a5f9ef8

Change profile name, delete suppreswarning

53ba8af

Update java/flight/flight-core/src/test/java/org/apache/arrow/flight/…

439403b

…TestClientMiddleware.java Co-authored-by: David Li <[email protected]>

lidavidm approved these changes Apr 29, 2022

View reviewed changes

Synch java developer doc

02d22c7

lidavidm reviewed Apr 29, 2022

View reviewed changes

docs/source/developers/java/building.rst Outdated Show resolved Hide resolved

Update docs/source/developers/java/building.rst

96d3c06

Co-authored-by: David Li <[email protected]>

lidavidm approved these changes May 2, 2022

View reviewed changes

lidavidm closed this in 760ad20 May 2, 2022

asfimport mentioned this pull request May 17, 2022

[Java] Support Java 17 #31200

Closed

		<groupId>commons-logging</groupId>
		<artifactId>commons-logging</artifactId>

		EXPECTED_BINARY_HEADERS.put("x-binary-bin", Arrays.asList(new byte[] {0}, new byte[]{1}));
		EXPECTED_TEXT_HEADERS.put("x-text", Arrays.asList("foo", "bar"));

ARROW-15755: [Java] Support Java 17 #12941

ARROW-15755: [Java] Support Java 17 #12941

Conversation

davisusanibar commented Apr 20, 2022

github-actions bot commented Apr 20, 2022

lidavidm commented Apr 21, 2022

lidavidm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Apr 21, 2022

lwhite1 commented Apr 21, 2022

davisusanibar commented Apr 21, 2022

lidavidm commented Apr 21, 2022

lidavidm commented Apr 21, 2022

lwhite1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulcd commented Apr 28, 2022

lidavidm commented Apr 28, 2022

Choose a reason for hiding this comment

raulcd commented Apr 28, 2022

davisusanibar commented Apr 28, 2022

lidavidm commented Apr 29, 2022

github-actions bot commented Apr 29, 2022

toddfarmer commented Apr 29, 2022

lidavidm commented Apr 29, 2022

davisusanibar commented Apr 29, 2022

pitrou commented May 2, 2022

ursabot commented May 6, 2022