Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
davisusanibar wants to merge 10 commits into from
+77 −25
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
acf79bd
Support Java 17
davisusanibar Apr 20, 2022
865b520
Solving JDK17 adapter/orc error: java.lang.ClassNotFoundException: or…
davisusanibar Apr 21, 2022
7a050e8
Merge branch 'master' into java-ARROW-15755
davisusanibar Apr 21, 2022
aea2f84
Solving PR comments
davisusanibar Apr 21, 2022
cdfae57
Solving PR comments
davisusanibar Apr 21, 2022
a5f9ef8
Merge branch 'master' into java-ARROW-15755
davisusanibar Apr 28, 2022
53ba8af
Change profile name, delete suppreswarning
davisusanibar Apr 28, 2022
439403b
Update java/flight/flight-core/src/test/java/org/apache/arrow/flight/…
davisusanibar Apr 28, 2022
02d22c7
Synch java developer doc
davisusanibar Apr 29, 2022
96d3c06
Update docs/source/developers/java/building.rst
davisusanibar Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/source/developers/java/building.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ Arrow Java uses the `Maven <https://maven.apache.org/>`_ build system.

Building requires:

* JDK 8, 9, 10, or 11, but only JDK 11 is tested in CI
* JDK 8, 9, 10, 11, 17, or 18, but only JDK 11 is tested in CI.
* Maven 3+

Building
Expand Down
19 changes: 18 additions & 1 deletion java/adapter/orc/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,27 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-client-runtime</artifactId>
<version>3.3.2</version>
<scope>test</scope>
<exclusions>
<exclusion>
<groupId>commons-logging</groupId>
<artifactId>commons-logging</artifactId>
Comment on lines +62 to +63
Copy link
Member

@lidavidm lidavidm Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@davisusanibar davisusanibar Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@lidavidm lidavidm Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@lidavidm lidavidm Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@davisusanibar davisusanibar Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-client-api</artifactId>
<version>3.3.2</version>
</dependency>
<dependency>
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-common</artifactId>
<version>2.2.0</version>
<version>3.3.2</version>
<scope>test</scope>
<exclusions>
<exclusion>
Expand Down
3 changes: 1 addition & 2 deletions java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestCallOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,7 @@ private void testHeaders(CallHeaders headers) {
FlightServer s =
FlightTestUtil.getStartedServer((location) -> FlightServer.builder(a, location, producer).build());
FlightClient client = FlightClient.builder(a, s.getLocation()).build()) {
client.doAction(new Action(""), new HeaderCallOption(headers)).hasNext();

Assert.assertFalse(client.doAction(new Action(""), new HeaderCallOption(headers)).hasNext());
final CallHeaders incomingHeaders = producer.headers();
for (String key : headers.keys()) {
if (key.endsWith(Metadata.BINARY_HEADER_SUFFIX)) {
Expand Down
13 changes: 7 additions & 6 deletions java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestClientMiddleware.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,12 +246,13 @@ public FlightClientMiddleware onCallStarted(CallInfo info) {
}

// Used to test that middleware can send and receive multi-valued text and binary headers.
static final Map<String, List<byte[]>> EXPECTED_BINARY_HEADERS = new HashMap<String, List<byte[]>>() {{
put("x-binary-bin", Arrays.asList(new byte[] {0}, new byte[]{1}));
}};
static final Map<String, List<String>> EXPECTED_TEXT_HEADERS = new HashMap<String, List<String>>() {{
put("x-text", Arrays.asList("foo", "bar"));
}};
static Map<String, List<byte[]>> EXPECTED_BINARY_HEADERS = new HashMap<String, List<byte[]>>();
static Map<String, List<String>> EXPECTED_TEXT_HEADERS = new HashMap<String, List<String>>();

static {
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"));
}

static class MultiHeaderServerMiddlewareFactory implements
FlightServerMiddleware.Factory<MultiHeaderServerMiddleware> {
Expand Down
6 changes: 3 additions & 3 deletions java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ public FlightInfo getPrimaryKeys(final TableRef tableRef, final CallOption... op
}

Objects.requireNonNull(tableRef.getTable());
builder.setTable(tableRef.getTable()).build();
builder.setTable(tableRef.getTable());

final FlightDescriptor descriptor = FlightDescriptor.command(Any.pack(builder.build()).toByteArray());
return client.getInfo(descriptor, options);
Expand All @@ -345,7 +345,7 @@ public FlightInfo getExportedKeys(final TableRef tableRef, final CallOption... o
}

Objects.requireNonNull(tableRef.getTable());
builder.setTable(tableRef.getTable()).build();
builder.setTable(tableRef.getTable());

final FlightDescriptor descriptor = FlightDescriptor.command(Any.pack(builder.build()).toByteArray());
return client.getInfo(descriptor, options);
Expand Down Expand Up @@ -373,7 +373,7 @@ public FlightInfo getImportedKeys(final TableRef tableRef,
}

Objects.requireNonNull(tableRef.getTable());
builder.setTable(tableRef.getTable()).build();
builder.setTable(tableRef.getTable());

final FlightDescriptor descriptor = FlightDescriptor.command(Any.pack(builder.build()).toByteArray());
return client.getInfo(descriptor, options);
Expand Down
7 changes: 4 additions & 3 deletions java/memory/memory-core/src/test/java/org/apache/arrow/memory/util/TestArrowBufPointer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.arrow.memory.util;

import static junit.framework.TestCase.assertEquals;
import static junit.framework.TestCase.assertNotSame;
import static junit.framework.TestCase.assertTrue;
import static org.junit.jupiter.api.Assertions.assertFalse;

Expand Down Expand Up @@ -120,18 +121,18 @@ public void testReuseHashCode() {
ArrowBufPointer pointer = new ArrowBufPointer(hasher);

pointer.set(buf, 0, 4);
pointer.hashCode();
int hashCode = pointer.hashCode();

// hash code computed
assertEquals(1, hasher.counter);

// no hash code re-compute
pointer.hashCode();
assertEquals(hashCode, pointer.hashCode());
assertEquals(1, hasher.counter);

// hash code re-computed
pointer.set(buf, 4, 4);
pointer.hashCode();
assertNotSame(hashCode, pointer.hashCode());
assertEquals(2, hasher.counter);
}
}
Expand Down
52 changes: 43 additions & 9 deletions java/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@
<forkCount>2</forkCount>
<checkstyle.failOnViolation>true</checkstyle.failOnViolation>
<errorprone.javac.version>9+181-r4173-1</errorprone.javac.version>
<error_prone_core.version>2.13.1</error_prone_core.version>
<maven-compiler-plugin.version>3.10.1</maven-compiler-plugin.version>
</properties>

<scm>
Expand Down Expand Up @@ -376,7 +378,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.6.2</version>
<version>${maven-compiler-plugin.version}</version>
<configuration>
<annotationProcessorPaths>
<path>
Expand Down Expand Up @@ -690,7 +692,7 @@

<profiles>
<profile>
<id>java-8</id>
<id>java-nodoclint</id>
<activation>
<jdk>[1.8,)</jdk>
</activation>
Expand Down Expand Up @@ -729,13 +731,14 @@
</profile>

<profile>
<id>error-prone</id>
<id>error-prone-jdk8</id>
<!--
Do not activate Error Prone while running with Eclipse/M2E as it causes incompatibilities
with other annotation processors.
See https://github.com/jbosstools/m2e-apt/issues/62 for details
-->
<activation>
<jdk>1.8</jdk>
Copy link
Member

@lidavidm lidavidm Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@davisusanibar davisusanibar Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

<property>
<name>!m2e.version</name>
</property>
Expand All @@ -746,9 +749,11 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<fork>true</fork>
<compilerArgs combine.children="append">
<arg>-XDcompilePolicy=simple</arg>
<arg>-Xplugin:ErrorProne</arg>
<arg>-J-Xbootclasspath/p:${settings.localRepository}/com/google/errorprone/javac/${errorprone.javac.version}/javac-${errorprone.javac.version}.jar</arg>
</compilerArgs>
<annotationProcessorPaths combine.children="append">
<path>
Expand All @@ -764,12 +769,11 @@
</profile>

<profile>
<id>error-prone-jdk8</id>
<!-- using github.com/google/error-prone-javac is required when running on JDK 8 -->
<id>error-prone-jdk11+</id>
<activation>
<jdk>1.8</jdk>
<jdk>[11,)</jdk>
<property>
<name>!m2e.version</name>
<name>!m2e.version</name>
</property>
</activation>
<build>
Expand All @@ -778,10 +782,40 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<fork>true</fork>
<source>8</source>
<target>8</target>
<encoding>UTF-8</encoding>
<compilerArgs combine.children="append">
<arg>-J-Xbootclasspath/p:${settings.localRepository}/com/google/errorprone/javac/${errorprone.javac.version}/javac-${errorprone.javac.version}.jar</arg>
<arg>-XDcompilePolicy=simple</arg>
<arg>
-Xplugin:ErrorProne \
-XepExcludedPaths:.*/(target/generated-sources)/.*
Copy link
Member

@lidavidm lidavidm Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@davisusanibar davisusanibar Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Member

@lidavidm lidavidm Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, weird.

</arg>
<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>
Comment on lines +794 to +803
Copy link

@hashhar hashhar Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Contributor Author

@davisusanibar davisusanibar Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two part on this question:

  1. 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+ ....)

  2. 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.

Copy link
Member

@lidavidm lidavidm Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add point (2) to the docs?

Copy link
Contributor Author

@davisusanibar davisusanibar Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@iamzafar iamzafar May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@davisusanibar davisusanibar May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</compilerArgs>
<annotationProcessorPaths combine.children="append">
<path>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_core</artifactId>
<version>${error_prone_core.version}</version>
</path>
</annotationProcessorPaths>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<argLine>--add-opens=java.base/java.nio=ALL-UNNAMED</argLine>
</configuration>
</plugin>
</plugins>
Expand Down