Skip to content
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.

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

ARROW-16328: [Java] Proposal to upgrade Arrow Java project to JPMS Java Platform Module System #13072

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
9493972
Initial draft to experiment with java module system
davisusanibar May 5, 2022
c652c12
Adding module-info.java for Arrow Java Memory / Vector
davisusanibar May 10, 2022
504c8c6
Adding initial module-info.java for Arrow Java Flight Core
davisusanibar May 10, 2022
a604275
Adding changes to the modules and to the ci new netty buffer patch
davisusanibar May 12, 2022
d3f7499
Solving build errors
davisusanibar May 12, 2022
1424d04
To solve error: Non-resolvable parent POM
davisusanibar May 12, 2022
fa791ec
Merge JPMS draft + main branch
davisusanibar Sep 26, 2022
057941d
JPMS update netty patch
davisusanibar Sep 26, 2022
ad4c485
Merge branch 'master' into java-ARROW-16328
davisusanibar Sep 26, 2022
ba1a9f6
Update librardy dependencies to the last version that support JPMS
davisusanibar Sep 28, 2022
c411828
Support JPMS with JRE<9
davisusanibar Sep 29, 2022
4dcfb04
Adding vector deps module
davisusanibar Sep 29, 2022
978a960
Merge with main branch
davisusanibar Sep 29, 2022
e445756
Delete tmp files
davisusanibar Sep 29, 2022
44c9cb2
Solving RAT errors
davisusanibar Sep 30, 2022
ff243ed
Add jar deps
davisusanibar Sep 30, 2022
67869ea
Remove not needed plugins
davisusanibar Sep 30, 2022
6ccb60d
Increase fork count
davisusanibar Sep 30, 2022
337e1ab
Reset changes
davisusanibar Sep 30, 2022
aa59489
Decouple io.netty.buffer to their own memory module
davisusanibar Sep 30, 2022
b929b7d
Change io.netty.buffer package name
davisusanibar Sep 30, 2022
1e9f1d6
Prepare different packages for JPMS
davisusanibar Oct 1, 2022
23758d5
Base project to start with JPMS
davisusanibar Oct 3, 2022
fbee610
Add JPMS for format & memory
davisusanibar Oct 3, 2022
d20a5a5
Add JPMS for vector
davisusanibar Oct 3, 2022
014808a
Clean code
davisusanibar Oct 3, 2022
a321083
Clean code
davisusanibar Oct 13, 2022
8087dc3
Merge branch 'master' into clean-code-before-pr
davisusanibar Oct 13, 2022
ab206c4
More clean code
davisusanibar Oct 13, 2022
1c560f5
Solving CData Interface testing errors
davisusanibar Oct 13, 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
6 changes: 6 additions & 0 deletions java/c/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@
<version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.arrow</groupId>
<artifactId>arrow-format</artifactId>
<version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
Expand Down
2 changes: 1 addition & 1 deletion java/flight/flight-core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-shade-plugin</artifactId>
<version>3.1.1</version>
<version>3.4.0</version>
<executions>
<execution>
<id>shade-main</id>
Expand Down
21 changes: 21 additions & 0 deletions java/format/src/main/java/module-info.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

module org.apache.arrow.format {
exports org.apache.arrow.flatbuf;
requires transitive flatbuffers.java;
}
28 changes: 28 additions & 0 deletions java/memory/memory-core/src/main/java/module-info.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

module org.apache.arrow.memory.core {
exports org.apache.arrow.memory;
exports org.apache.arrow.memory.rounding;
exports org.apache.arrow.memory.util;
exports org.apache.arrow.memory.util.hash;
exports org.apache.arrow.util;
requires transitive jdk.unsupported;
requires jsr305;
requires org.immutables.value;
requires slf4j.api;
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,13 @@
*/
final class CheckAllocator {
private static final Logger logger = LoggerFactory.getLogger(CheckAllocator.class);
private static final String ALLOCATOR_PATH = "org/apache/arrow/memory/DefaultAllocationManagerFactory.class";
// unique package names needed by JPMS module naming
private static final String ALLOCATOR_PATH_CORE =
"org/apache/arrow/memory/DefaultAllocationManagerFactory.class";
private static final String ALLOCATOR_PATH_UNSAFE =
"org/apache/arrow/memory/unsafe/DefaultAllocationManagerFactory.class";
private static final String ALLOCATOR_PATH_NETTY =
"org/apache/arrow/memory/netty/DefaultAllocationManagerFactory.class";

private CheckAllocator() {

Expand All @@ -41,7 +47,18 @@ static String check() {
Set<URL> urls = scanClasspath();
URL rootAllocator = assertOnlyOne(urls);
reportResult(rootAllocator);
return "org.apache.arrow.memory.DefaultAllocationManagerFactory";
if (rootAllocator.getPath().contains("memory-core") ||
rootAllocator.getPath().contains("/org/apache/arrow/memory/core/")) {
return "org.apache.arrow.memory.DefaultAllocationManagerFactory";
} else if (rootAllocator.getPath().contains("memory-unsafe") ||
rootAllocator.getPath().contains("/org/apache/arrow/memory/unsafe/")) {
return "org.apache.arrow.memory.unsafe.DefaultAllocationManagerFactory";
} else if (rootAllocator.getPath().contains("memory-netty") ||
rootAllocator.getPath().contains("/org/apache/arrow/memory/netty/")) {
return "org.apache.arrow.memory.netty.DefaultAllocationManagerFactory";
} else {
throw new IllegalStateException("Unknown allocation manager type to infer. Current: " + rootAllocator.getPath());
}
}


Expand All @@ -53,9 +70,21 @@ private static Set<URL> scanClasspath() {
ClassLoader allocatorClassLoader = CheckAllocator.class.getClassLoader();
Enumeration<URL> paths;
if (allocatorClassLoader == null) {
paths = ClassLoader.getSystemResources(ALLOCATOR_PATH);
paths = ClassLoader.getSystemResources(ALLOCATOR_PATH_CORE);
if (!paths.hasMoreElements()) {
paths = ClassLoader.getSystemResources(ALLOCATOR_PATH_UNSAFE);
}
if (!paths.hasMoreElements()) {
paths = ClassLoader.getSystemResources(ALLOCATOR_PATH_NETTY);
}
} else {
paths = allocatorClassLoader.getResources(ALLOCATOR_PATH);
paths = allocatorClassLoader.getResources(ALLOCATOR_PATH_CORE);
if (!paths.hasMoreElements()) {
paths = allocatorClassLoader.getResources(ALLOCATOR_PATH_UNSAFE);
}
if (!paths.hasMoreElements()) {
paths = allocatorClassLoader.getResources(ALLOCATOR_PATH_NETTY);
}
}
while (paths.hasMoreElements()) {
URL path = paths.nextElement();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

import java.lang.reflect.Field;

import org.apache.arrow.util.VisibleForTesting;

/**
* A class for choosing the default allocation manager.
*/
Expand Down Expand Up @@ -61,7 +63,11 @@ public enum AllocationManagerType {
Unknown,
}

static AllocationManagerType getDefaultAllocationManagerType() {
/**
* JPMS needed.
*/
@VisibleForTesting
public static AllocationManagerType getDefaultAllocationManagerType() {
AllocationManagerType ret = AllocationManagerType.Unknown;

try {
Expand Down Expand Up @@ -115,7 +121,7 @@ private static AllocationManager.Factory getFactory(String clazzName) {

private static AllocationManager.Factory getUnsafeFactory() {
try {
return getFactory("org.apache.arrow.memory.UnsafeAllocationManager");
return getFactory("org.apache.arrow.memory.unsafe.UnsafeAllocationManager");
} catch (RuntimeException e) {
throw new RuntimeException("Please add arrow-memory-unsafe to your classpath," +
" No DefaultAllocationManager found to instantiate an UnsafeAllocationManager", e);
Expand All @@ -124,7 +130,7 @@ private static AllocationManager.Factory getUnsafeFactory() {

private static AllocationManager.Factory getNettyFactory() {
try {
return getFactory("org.apache.arrow.memory.NettyAllocationManager");
return getFactory("org.apache.arrow.memory.netty.NettyAllocationManager");
} catch (RuntimeException e) {
throw new RuntimeException("Please add arrow-memory-netty to your classpath," +
" No DefaultAllocationManager found to instantiate an NettyAllocationManager", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ public void testRootAllocator_closeWithOutstanding() throws Exception {
}

@Test
@Ignore //TODO: Enable test impacted by JPMS implementation
public void testRootAllocator_getEmpty() throws Exception {
try (final RootAllocator rootAllocator =
new RootAllocator(MAX_ALLOCATION)) {
Expand Down Expand Up @@ -1070,7 +1071,7 @@ public void testAllocator_claimedReservation() throws Exception {
public void testInitReservationAndLimit() throws Exception {
try (final RootAllocator rootAllocator = new RootAllocator(MAX_ALLOCATION)) {
try (final BufferAllocator childAllocator = rootAllocator.newChildAllocator(
"child", 2048, 4096)) {
"child", 2048, 4096)) {
assertEquals(2048, childAllocator.getInitReservation());
assertEquals(4096, childAllocator.getLimit());
}
Expand Down
45 changes: 45 additions & 0 deletions java/memory/memory-netty-buffer-patch/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?xml version="1.0" encoding="UTF-8"?>
<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
license agreements. See the NOTICE file distributed with this work for additional
information regarding copyright ownership. The ASF licenses this file to
You under the Apache License, Version 2.0 (the "License"); you may not use
this file except in compliance with the License. You may obtain a copy of
the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required
by applicable law or agreed to in writing, software distributed under the
License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
OF ANY KIND, either express or implied. See the License for the specific
language governing permissions and limitations under the License. -->
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<parent>
<artifactId>arrow-memory</artifactId>
<groupId>org.apache.arrow</groupId>
<version>10.0.0-SNAPSHOT</version>
</parent>
<modelVersion>4.0.0</modelVersion>

<artifactId>arrow-memory-netty-buffer-patch</artifactId>
<name>Arrow Memory - Netty Buffer</name>
<description>Netty Buffer needed to patch that is consumed by Arrow Memory Netty</description>

<dependencies>
<dependency>
<groupId>org.apache.arrow</groupId>
<artifactId>arrow-memory-core</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-buffer</artifactId>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-common</artifactId>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
</dependencies>
</project>
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@

package io.netty.buffer;

import static org.apache.arrow.memory.util.LargeMemoryUtil.checkedCastToInt;

import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand All @@ -29,10 +27,12 @@
import java.nio.channels.ScatteringByteChannel;

import org.apache.arrow.memory.ArrowBuf;
import org.apache.arrow.memory.ArrowByteBufAllocator;
import org.apache.arrow.memory.BoundsChecking;
import org.apache.arrow.memory.BufferAllocator;
import org.apache.arrow.memory.patch.ArrowByteBufAllocator;
import org.apache.arrow.memory.util.LargeMemoryUtil;
import org.apache.arrow.util.Preconditions;
import org.apache.arrow.util.VisibleForTesting;

import io.netty.util.internal.PlatformDependent;

Expand Down Expand Up @@ -257,7 +257,7 @@ public ByteBuffer nioBuffer(long index, int length) {
* @return ByteBuffer
*/
private ByteBuffer getDirectBuffer(long index) {
return PlatformDependent.directBuffer(addr(index), checkedCastToInt(length - index));
return PlatformDependent.directBuffer(addr(index), LargeMemoryUtil.checkedCastToInt(length - index));
}

@Override
Expand Down Expand Up @@ -573,6 +573,7 @@ public NettyArrowBuf setMedium(int index, int value) {
}

@Override
@VisibleForTesting
protected void _setInt(int index, int value) {
setInt(index, value);
}
Expand All @@ -590,6 +591,7 @@ public NettyArrowBuf setInt(int index, int value) {
}

@Override
@VisibleForTesting
protected void _setLong(int index, long value) {
setLong(index, value);
}
Expand All @@ -613,9 +615,9 @@ public static NettyArrowBuf unwrapBuffer(ArrowBuf buf) {
final NettyArrowBuf nettyArrowBuf = new NettyArrowBuf(
buf,
buf.getReferenceManager().getAllocator(),
checkedCastToInt(buf.capacity()));
nettyArrowBuf.readerIndex(checkedCastToInt(buf.readerIndex()));
nettyArrowBuf.writerIndex(checkedCastToInt(buf.writerIndex()));
LargeMemoryUtil.checkedCastToInt(buf.capacity()));
nettyArrowBuf.readerIndex(LargeMemoryUtil.checkedCastToInt(buf.readerIndex()));
nettyArrowBuf.writerIndex(LargeMemoryUtil.checkedCastToInt(buf.writerIndex()));
return nettyArrowBuf;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,12 @@

package io.netty.buffer;

import static org.apache.arrow.memory.util.AssertionUtil.ASSERT_ENABLED;

import java.lang.reflect.Field;
import java.nio.ByteBuffer;
import java.util.concurrent.atomic.AtomicLong;

import org.apache.arrow.memory.OutOfMemoryException;
import org.apache.arrow.memory.util.AssertionUtil;
import org.apache.arrow.memory.util.LargeMemoryUtil;

import io.netty.util.internal.OutOfDirectMemoryError;
Expand Down Expand Up @@ -183,7 +182,7 @@ private UnsafeDirectLittleEndian newDirectBufferL(int initialCapacity, int maxCa
fail();
}

if (!ASSERT_ENABLED) {
if (!AssertionUtil.ASSERT_ENABLED) {
return new UnsafeDirectLittleEndian((PooledUnsafeDirectByteBuf) buf);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
* limitations under the License.
*/

package org.apache.arrow.memory;
package org.apache.arrow.memory.patch;

import org.apache.arrow.memory.BufferAllocator;

import io.netty.buffer.AbstractByteBufAllocator;
import io.netty.buffer.ByteBuf;
Expand Down
27 changes: 27 additions & 0 deletions java/memory/memory-netty/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,15 @@
<artifactId>arrow-memory-core</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.apache.arrow</groupId>
<artifactId>arrow-memory-netty-buffer-patch</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-buffer</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>io.netty</groupId>
Expand All @@ -38,6 +44,7 @@
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.immutables</groupId>
Expand Down Expand Up @@ -68,5 +75,25 @@
</plugins>
</build>
</profile>

<profile>
<id>memory-jdk11+</id>
<activation>
<jdk>[11,]</jdk>
</activation>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<compilerArgs combine.children="append">
<arg>--patch-module=io.netty.buffer=${project.basedir}/../memory-netty-buffer-patch/target/arrow-memory-netty-buffer-patch-${project.version}.jar</arg>
Copy link
Member

Choose a reason for hiding this comment

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

@davisusanibar , I'm having problems getting this step to run in the new PR and am not entirely sure if it is necessary.

The new PR just does a pure syntax compilation of module-info.java so it doesn't verify the dependencies listed and just uses the classpath to look for classes from arrow-memory-netty-buffer-patch.

Was this step added so that this module would correctly find classes in io.netty.buffer defined in arrow-memory-netty-buffer-patch at build-time only, or does this have an effect at run time? My reading online is that even if we used patch-module to compile, a user utilizing the module would still need to use patch-module to specify module extensions to netty.

If patch-module is only used here to help get this JAR to compile, it seems like it isn't necessary in the new PR since the plugin is compiling module-info.java without verifying its imports.

</compilerArgs>
</configuration>
</plugin>
</plugins>
</build>
</profile>
</profiles>
</project>
Loading