You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/07/02 16:32:26 UTC
[GitHub] [arrow] rymurr opened a new pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
rymurr opened a new pull request #7619:
URL: https://github.com/apache/arrow/pull/7619
This finishes the netty split in arrow-memory and creates 3 new modules
* memory-core: core memory implementation
* memory-netty: netty allocation manager
* memory-unsafe: unsafe allocation manager
The bulk of the changes here are moving files and adding the correct dependencies to poms
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451386346
##########
File path: java/memory/memory-core/pom.xml
##########
@@ -0,0 +1,65 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-core</artifactId>
+
+ <name>Arrow Memory - Core</name>
+ <description>Core off-heap memory management libraries for Arrow ValueVectors.</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>com.google.code.findbugs</groupId>
+ <artifactId>jsr305</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value</artifactId>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-653114882
https://issues.apache.org/jira/browse/ARROW-9300
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451918970
##########
File path: java/performance/pom.xml
##########
@@ -53,7 +53,7 @@
</dependency>
<dependency>
<groupId>org.apache.arrow</groupId>
- <artifactId>arrow-memory</artifactId>
+ <artifactId>arrow-memory-core</artifactId>
Review comment:
We also need the dependency of arrow-memory-netty here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r452341314
##########
File path: java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java
##########
@@ -114,10 +114,20 @@ static AllocationManagerType getDefaultAllocationManagerType() {
}
private static AllocationManager.Factory getUnsafeFactory() {
- return getFactory("org.apache.arrow.memory.UnsafeAllocationManager");
+ try {
+ return getFactory("org.apache.arrow.memory.UnsafeAllocationManager");
+ } catch (RuntimeException e) {
+ throw new RuntimeException("Please add arrow-memory-unsafe to your classpath," +
+ " No DefaultAllocationManager found to instantiate an UnsafeAllocationManager", e);
+ }
}
private static AllocationManager.Factory getNettyFactory() {
- return getFactory("org.apache.arrow.memory.NettyAllocationManager");
+ try {
+ return getFactory("org.apache.arrow.memory.NettyAllocationManager");
+ } catch (RuntimeException e) {
+ throw new RuntimeException("Please add arrow-memory-unsafe to your classpath," +
Review comment:
should be `arrow-memory-netty` and `NettyAllocationManager` below
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r449540176
##########
File path: java/memory/memory-core/src/test/java/org/apache/arrow/memory/DefaultAllocationManagerFactory.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.memory;
+
+import org.apache.arrow.memory.util.MemoryUtil;
+
+/**
+ * The default Allocation Manager Factory for a module.
+ *
+ * This will be split out to the arrow-memory-netty/arrow-memory-unsafe modules
+ * as part of ARROW-8230. This is currently a placeholder which defaults to Netty.
+ *
+ */
+public class DefaultAllocationManagerFactory implements AllocationManager.Factory {
+
+ public static final AllocationManager.Factory FACTORY = new DefaultAllocationManagerFactory();
+
+ @Override
+ public AllocationManager create(BaseAllocator accountingAllocator, long size) {
+ return new AllocationManager(accountingAllocator) {
+ private final long allocatedSize = size;
+ private final long address = MemoryUtil.UNSAFE.allocateMemory(size);
+
+ @Override
+ public long getSize() {
+ return allocatedSize;
+ }
+
+ @Override
+ protected long memoryAddress() {
+ return address;
+ }
+
+ @Override
+ protected void release0() {
+ MemoryUtil.UNSAFE.freeMemory(address);
+ }
+ };
+ }
+
+ @Override
+ public ArrowBuf empty() {
Review comment:
This is only in tests, but I agree. Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-653520801
> Thanks for the comments @liyafan82 . I have updated and rebased to pull in your fix from #7628
Sorry for the trouble, and thank you for the effort.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler closed pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
BryanCutler closed pull request #7619:
URL: https://github.com/apache/arrow/pull/7619
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-655401626
Hey all,
Thanks @BryanCutler and @liyafan82 for the comments and thanks @jacques-n for the reasoning behind unsafe/netty split.
I have addressed your comments in the most recent update and have created two new tickets:
1. bump netty version: https://issues.apache.org/jira/browse/ARROW-9370
2. run tests twice: https://issues.apache.org/jira/browse/ARROW-9371
I will raise PRs for these soon
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451102944
##########
File path: java/adapter/orc/pom.xml
##########
@@ -15,10 +15,16 @@
<dependencies>
<dependency>
<groupId>org.apache.arrow</groupId>
- <artifactId>arrow-memory</artifactId>
- <version>${project.version}</version>
+ <artifactId>arrow-memory-core</artifactId>
+ <version>1.0.0-SNAPSHOT</version>
Review comment:
was this intentional?
##########
File path: java/memory/memory-core/pom.xml
##########
@@ -0,0 +1,65 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-core</artifactId>
+
+ <name>Arrow Memory - Core</name>
+ <description>Core off-heap memory management libraries for Arrow ValueVectors.</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>com.google.code.findbugs</groupId>
+ <artifactId>jsr305</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value</artifactId>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
Review comment:
is this necessary here?
##########
File path: java/vector/pom.xml
##########
@@ -50,6 +50,12 @@
<artifactId>commons-codec</artifactId>
<version>1.10</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.arrow</groupId>
+ <artifactId>arrow-memory-netty</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
Maybe we should run the unit tests once for each of the 2 allocators?
##########
File path: java/vector/pom.xml
##########
@@ -50,6 +50,12 @@
<artifactId>commons-codec</artifactId>
<version>1.10</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.arrow</groupId>
+ <artifactId>arrow-memory-netty</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-buffer</artifactId>
Review comment:
This could be removed now right?
##########
File path: java/memory/memory-netty/pom.xml
##########
@@ -0,0 +1,107 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-netty</artifactId>
+ <name>Arrow Memory - Netty</name>
+ <description>Netty allocator and utils for allocating memory in Arrow</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>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value</artifactId>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
+ <arrow.vector.max_allocation_bytes>1048576</arrow.vector.max_allocation_bytes>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ <!-- Note: changing the below configuration might increase the max allocation size for a vector
+ which in turn can cause OOM. -->
+ <argLine>-Darrow.vector.max_allocation_bytes=1048576</argLine>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
+ <profiles>
+ <profile>
+ <!-- This profile turns on integration testing. It activates the failsafe plugin and will run any tests
+ with the 'IT' prefix. This should be run in a separate CI build or on developers machines as it potentially
+ uses quite a bit of memory. Activate the tests by adding -Pintegration-tests to your maven command line -->
+ <id>integration-tests</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-failsafe-plugin</artifactId>
+ <configuration>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ <argLine></argLine>
+ </configuration>
+ <executions>
+ <execution>
+ <goals>
+ <goal>integration-test</goal>
+ <goal>verify</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ </profiles>
+
+</project>
Review comment:
add newline?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-653514629
Thanks for the comments @liyafan82 . I have updated and rebased to pull in your fix from #7628
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r449494433
##########
File path: java/vector/pom.xml
##########
@@ -50,6 +50,12 @@
<artifactId>commons-codec</artifactId>
<version>1.10</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.arrow</groupId>
+ <artifactId>arrow-memory-netty</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
Maybe we also need the memory-unsafe dependency here?
Otherwise, our integeration tests will not be able to pass.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r450041982
##########
File path: java/memory/memory-core/src/test/java/org/apache/arrow/memory/DefaultAllocationManagerFactory.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.memory;
+
+import org.apache.arrow.memory.util.MemoryUtil;
+
+/**
+ * The default Allocation Manager Factory for a module.
+ *
+ * This will be split out to the arrow-memory-netty/arrow-memory-unsafe modules
+ * as part of ARROW-8230. This is currently a placeholder which defaults to Netty.
+ *
+ */
+public class DefaultAllocationManagerFactory implements AllocationManager.Factory {
Review comment:
We have 3 classes in the code base named "DefaultAllocationManagerFactory". Do we need all of them?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r449492262
##########
File path: java/memory/memory-core/src/test/java/org/apache/arrow/memory/DefaultAllocationManagerFactory.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.memory;
+
+import org.apache.arrow.memory.util.MemoryUtil;
+
+/**
+ * The default Allocation Manager Factory for a module.
+ *
+ * This will be split out to the arrow-memory-netty/arrow-memory-unsafe modules
+ * as part of ARROW-8230. This is currently a placeholder which defaults to Netty.
+ *
+ */
+public class DefaultAllocationManagerFactory implements AllocationManager.Factory {
+
+ public static final AllocationManager.Factory FACTORY = new DefaultAllocationManagerFactory();
+
+ @Override
+ public AllocationManager create(BaseAllocator accountingAllocator, long size) {
+ return new AllocationManager(accountingAllocator) {
+ private final long allocatedSize = size;
+ private final long address = MemoryUtil.UNSAFE.allocateMemory(size);
+
+ @Override
+ public long getSize() {
+ return allocatedSize;
+ }
+
+ @Override
+ protected long memoryAddress() {
+ return address;
+ }
+
+ @Override
+ protected void release0() {
+ MemoryUtil.UNSAFE.freeMemory(address);
+ }
+ };
+ }
+
+ @Override
+ public ArrowBuf empty() {
Review comment:
Since this is a frequently called method, I think it would be beneficial to reuse the empty ArrowBuf.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r450071734
##########
File path: java/memory/memory-core/src/test/java/org/apache/arrow/memory/DefaultAllocationManagerFactory.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.memory;
+
+import org.apache.arrow.memory.util.MemoryUtil;
+
+/**
+ * The default Allocation Manager Factory for a module.
+ *
+ * This will be split out to the arrow-memory-netty/arrow-memory-unsafe modules
+ * as part of ARROW-8230. This is currently a placeholder which defaults to Netty.
+ *
+ */
+public class DefaultAllocationManagerFactory implements AllocationManager.Factory {
Review comment:
Hey @liyafan82, yes i think we do:
1) lives in the netty module.
2) lives in the unsafe module
3) lives in the tests portion of the core memory module.
On startup we first check if a specific memory allocator has been requested. If yes we instantiate that. If no specific impl has been requested then we search the classpath for a `DefaultAllocationManagerFactory`. If we find one then we instantiate it, else we throw a Runtime exception (can't continue without an allocation manager).
So each concrete impl of `AllocationManager` requires a factory to create it and we have a trivial allocation manager in `arrow-memory-core` tests to help the core tests finish.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r449559785
##########
File path: java/vector/pom.xml
##########
@@ -50,6 +50,12 @@
<artifactId>commons-codec</artifactId>
<version>1.10</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.arrow</groupId>
+ <artifactId>arrow-memory-netty</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
I believe the netty allocator can allocate 64-bit spaces but netty buffers can't directly reference them?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jacques-n commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-655210526
> Looks fine for the most part, but I'm not really sure why we need to separate `arrow-memory-core` and `arrow-memory-unsafe`? Couldn't those be combined since it wouldn't add any other dependencies, and that would also simplify things. Plus, it doesn't really make sense to have a module `arrow-memory-core` without a default allocator that would probably build fine with `arrow-vector`, but then blow up at runtime. What do you think @rymurr and @liyafan82 ?
This is modeled after the slf4j pattern where the logging implementation is separated from the core apis. That way the default pattern can people sourcing the desired allocator via dependency without having to configure one. This seems much cleaner that the previous approaches where people had to manually configure or override via system properties.
Additionally, I'd add that for new users I think we would suggest using the Netty one, not the unsafe one. It is much more complete/comprehensive and intelligent. So having a default implementation that we always tell people to override seems worse than they having a hard failure if they don't source any. If we want to make things easier, we could also introduce some vector + allocator depedency poms and then use those in documentation, etc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-656296147
merged to master, thanks @rymurr !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451381837
##########
File path: java/memory/memory-netty/pom.xml
##########
@@ -0,0 +1,107 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-netty</artifactId>
+ <name>Arrow Memory - Netty</name>
+ <description>Netty allocator and utils for allocating memory in Arrow</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>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value</artifactId>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
+ <arrow.vector.max_allocation_bytes>1048576</arrow.vector.max_allocation_bytes>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ <!-- Note: changing the below configuration might increase the max allocation size for a vector
+ which in turn can cause OOM. -->
+ <argLine>-Darrow.vector.max_allocation_bytes=1048576</argLine>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
+ <profiles>
+ <profile>
+ <!-- This profile turns on integration testing. It activates the failsafe plugin and will run any tests
+ with the 'IT' prefix. This should be run in a separate CI build or on developers machines as it potentially
+ uses quite a bit of memory. Activate the tests by adding -Pintegration-tests to your maven command line -->
+ <id>integration-tests</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-failsafe-plugin</artifactId>
+ <configuration>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ <argLine></argLine>
+ </configuration>
+ <executions>
+ <execution>
+ <goals>
+ <goal>integration-test</goal>
+ <goal>verify</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ </profiles>
+
+</project>
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-655246147
On a related note, it seems like our netty version 4.1.27 is pretty old now, ~2 years, do you all think it would be good to upgrade this before the 1.0.0 release? It looks like there is a security vulnerability pre 4.1.44 https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-20445
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r452076588
##########
File path: java/memory/memory-core/src/test/java/org/apache/arrow/memory/DefaultAllocationManagerFactory.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.memory;
+
+import org.apache.arrow.memory.util.MemoryUtil;
+
+/**
+ * The default Allocation Manager Factory for a module.
+ *
+ * This will be split out to the arrow-memory-netty/arrow-memory-unsafe modules
+ * as part of ARROW-8230. This is currently a placeholder which defaults to Netty.
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-655244616
I agree that the recommended allocator should still be the netty one for now, so I guess it wouldn't be good to bundle the unsafe allocator as a possible default. I'm good with raising an error then if a default allocator is not on the classpath, as long as it's clear to the user what they need to do. Right now it looks like the error is:
```
java.lang.RuntimeException: No DefaultAllocationManager found on classpath. Can't allocate Arrow buffers.
```
@rymurr would you mind adding something to the message like "it is recommended to add `arrow-memory-netty` or `arrow-memory-unsafe` as a dependency to provide a `DefaultAllocatorManager"?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr edited a comment on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr edited a comment on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-655401626
Hey all,
Thanks @BryanCutler and @liyafan82 for the comments and thanks @jacques-n for the reasoning behind unsafe/netty module split.
I have addressed your comments in the most recent update and have created two new tickets:
1. bump netty version: https://issues.apache.org/jira/browse/ARROW-9370
2. run tests twice: https://issues.apache.org/jira/browse/ARROW-9371
I will raise PRs for these soon
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451735636
##########
File path: java/memory/memory-core/pom.xml
##########
@@ -0,0 +1,60 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-core</artifactId>
+
+ <name>Arrow Memory - Core</name>
+ <description>Core off-heap memory management libraries for Arrow ValueVectors.</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>com.google.code.findbugs</groupId>
+ <artifactId>jsr305</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value</artifactId>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+</project>
Review comment:
I have added a new line in all 3 locations
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451734778
##########
File path: java/memory/memory-unsafe/pom.xml
##########
@@ -0,0 +1,54 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-unsafe</artifactId>
+ <name>Arrow Memory - Unsafe</name>
+ <description>Allocator and utils for allocating memory in Arrow based on sun.misc.Unsafe</description>
+
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.arrow</groupId>
+ <artifactId>arrow-memory-core</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
+</project>
Review comment:
How come you want a newline? I notice that most/all poms have it but it isn't picked up by the style checker. Not a problem either way for me, just curious
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r452076883
##########
File path: java/memory/memory-netty/pom.xml
##########
@@ -0,0 +1,106 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-netty</artifactId>
+ <name>Arrow Memory - Netty</name>
+ <description>Netty allocator and utils for allocating memory in Arrow</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>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value</artifactId>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
+ <arrow.vector.max_allocation_bytes>1048576</arrow.vector.max_allocation_bytes>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ <!-- Note: changing the below configuration might increase the max allocation size for a vector
+ which in turn can cause OOM. -->
+ <argLine>-Darrow.vector.max_allocation_bytes=1048576</argLine>
Review comment:
agreed, removed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451381217
##########
File path: java/adapter/orc/pom.xml
##########
@@ -15,10 +15,16 @@
<dependencies>
<dependency>
<groupId>org.apache.arrow</groupId>
- <artifactId>arrow-memory</artifactId>
- <version>${project.version}</version>
+ <artifactId>arrow-memory-core</artifactId>
+ <version>1.0.0-SNAPSHOT</version>
Review comment:
no, don't know how that snuck through. Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451386610
##########
File path: java/memory/memory-core/pom.xml
##########
@@ -0,0 +1,65 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-core</artifactId>
+
+ <name>Arrow Memory - Core</name>
+ <description>Core off-heap memory management libraries for Arrow ValueVectors.</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>com.google.code.findbugs</groupId>
+ <artifactId>jsr305</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value</artifactId>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
+ <arrow.vector.max_allocation_bytes>1048576</arrow.vector.max_allocation_bytes>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ <!-- Note: changing the below configuration might increase the max allocation size for a vector
+ which in turn can cause OOM. -->
+ <argLine>-Darrow.vector.max_allocation_bytes=1048576</argLine>
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451405221
##########
File path: java/vector/pom.xml
##########
@@ -50,6 +50,12 @@
<artifactId>commons-codec</artifactId>
<version>1.10</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.arrow</groupId>
+ <artifactId>arrow-memory-netty</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-buffer</artifactId>
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451916739
##########
File path: java/memory/memory-core/src/test/java/org/apache/arrow/memory/DefaultAllocationManagerFactory.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.
+ */
+
+package org.apache.arrow.memory;
+
+import org.apache.arrow.memory.util.MemoryUtil;
+
+/**
+ * The default Allocation Manager Factory for a module.
+ *
+ * This will be split out to the arrow-memory-netty/arrow-memory-unsafe modules
+ * as part of ARROW-8230. This is currently a placeholder which defaults to Netty.
Review comment:
I think this comment can be removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-655716943
> Just a minor nit on making sure there are newlines at the end of the new pom.xml files for consistency. Also, would you mind improving the error message if no` DefaultAllocationManagerFactor` mentioned at [#7619 (comment)](https://github.com/apache/arrow/pull/7619#issuecomment-655244616)? Otherwise LGTM
Both are done now. I mistakenly missed the ` DefaultAllocationManagerFactory` update in the previous change set
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451710176
##########
File path: java/memory/memory-netty/pom.xml
##########
@@ -0,0 +1,107 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-netty</artifactId>
+ <name>Arrow Memory - Netty</name>
+ <description>Netty allocator and utils for allocating memory in Arrow</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>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value</artifactId>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
+ <arrow.vector.max_allocation_bytes>1048576</arrow.vector.max_allocation_bytes>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ <!-- Note: changing the below configuration might increase the max allocation size for a vector
+ which in turn can cause OOM. -->
+ <argLine>-Darrow.vector.max_allocation_bytes=1048576</argLine>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
+ <profiles>
+ <profile>
+ <!-- This profile turns on integration testing. It activates the failsafe plugin and will run any tests
+ with the 'IT' prefix. This should be run in a separate CI build or on developers machines as it potentially
+ uses quite a bit of memory. Activate the tests by adding -Pintegration-tests to your maven command line -->
+ <id>integration-tests</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-failsafe-plugin</artifactId>
+ <configuration>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ <argLine></argLine>
+ </configuration>
+ <executions>
+ <execution>
+ <goals>
+ <goal>integration-test</goal>
+ <goal>verify</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+ </profile>
+ </profiles>
+
+</project>
Review comment:
it still looks like it needs a newline at the end of the file
##########
File path: java/memory/memory-unsafe/pom.xml
##########
@@ -0,0 +1,54 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-unsafe</artifactId>
+ <name>Arrow Memory - Unsafe</name>
+ <description>Allocator and utils for allocating memory in Arrow based on sun.misc.Unsafe</description>
+
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.arrow</groupId>
+ <artifactId>arrow-memory-core</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
+</project>
Review comment:
newline here
##########
File path: java/memory/memory-core/pom.xml
##########
@@ -0,0 +1,60 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-core</artifactId>
+
+ <name>Arrow Memory - Core</name>
+ <description>Core off-heap memory management libraries for Arrow ValueVectors.</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>com.google.code.findbugs</groupId>
+ <artifactId>jsr305</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value</artifactId>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+</project>
Review comment:
newline here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451235584
##########
File path: java/memory/memory-core/pom.xml
##########
@@ -0,0 +1,65 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-core</artifactId>
+
+ <name>Arrow Memory - Core</name>
+ <description>Core off-heap memory management libraries for Arrow ValueVectors.</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>com.google.code.findbugs</groupId>
+ <artifactId>jsr305</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value</artifactId>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
+ <arrow.vector.max_allocation_bytes>1048576</arrow.vector.max_allocation_bytes>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ <!-- Note: changing the below configuration might increase the max allocation size for a vector
+ which in turn can cause OOM. -->
+ <argLine>-Darrow.vector.max_allocation_bytes=1048576</argLine>
Review comment:
This seems duplicate with the system property defined above
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-655232313
> Looks fine for the most part, but I'm not really sure why we need to separate `arrow-memory-core` and `arrow-memory-unsafe`? Couldn't those be combined since it wouldn't add any other dependencies, and that would also simplify things. Plus, it doesn't really make sense to have a module `arrow-memory-core` without a default allocator that would probably build fine with `arrow-vector`, but then blow up at runtime. What do you think @rymurr and @liyafan82 ?
@BryanCutler I agree with you that it makes things simpler.
Since we may need to continue to use netty implementation as the default one (as indicated by @jacques-n ), maybe it is beneficial to keep the arrow-memory-unsafe module, at least for now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r449546201
##########
File path: java/vector/pom.xml
##########
@@ -50,6 +50,12 @@
<artifactId>commons-codec</artifactId>
<version>1.10</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.arrow</groupId>
+ <artifactId>arrow-memory-netty</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
I ran `mvn clean install -Pintegration-tests` and it was clean. So the integration tests went ok w/ Netty
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r449553368
##########
File path: java/vector/pom.xml
##########
@@ -50,6 +50,12 @@
<artifactId>commons-codec</artifactId>
<version>1.10</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.arrow</groupId>
+ <artifactId>arrow-memory-netty</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
Thanks for your feedback.
It is weird. I thought only unsafe allocator supports allocating a buffer larger than 2GB, because the netty allocator uses int32 internally.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r449540606
##########
File path: java/vector/pom.xml
##########
@@ -50,6 +50,12 @@
<artifactId>commons-codec</artifactId>
<version>1.10</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.arrow</groupId>
+ <artifactId>arrow-memory-netty</artifactId>
+ <version>${project.version}</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
I don't think the integration tests use UnsafeAllocator do they?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r452356610
##########
File path: java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java
##########
@@ -114,10 +114,20 @@ static AllocationManagerType getDefaultAllocationManagerType() {
}
private static AllocationManager.Factory getUnsafeFactory() {
- return getFactory("org.apache.arrow.memory.UnsafeAllocationManager");
+ try {
+ return getFactory("org.apache.arrow.memory.UnsafeAllocationManager");
+ } catch (RuntimeException e) {
+ throw new RuntimeException("Please add arrow-memory-unsafe to your classpath," +
+ " No DefaultAllocationManager found to instantiate an UnsafeAllocationManager", e);
+ }
}
private static AllocationManager.Factory getNettyFactory() {
- return getFactory("org.apache.arrow.memory.NettyAllocationManager");
+ try {
+ return getFactory("org.apache.arrow.memory.NettyAllocationManager");
+ } catch (RuntimeException e) {
+ throw new RuntimeException("Please add arrow-memory-unsafe to your classpath," +
Review comment:
sigh....sorry. Copy paste for the win. Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451917617
##########
File path: java/memory/memory-netty/pom.xml
##########
@@ -0,0 +1,106 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-netty</artifactId>
+ <name>Arrow Memory - Netty</name>
+ <description>Netty allocator and utils for allocating memory in Arrow</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>
+ <dependency>
+ <groupId>org.immutables</groupId>
+ <artifactId>value</artifactId>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <io.netty.tryReflectionSetAccessible>true</io.netty.tryReflectionSetAccessible>
+ <arrow.vector.max_allocation_bytes>1048576</arrow.vector.max_allocation_bytes>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ <!-- Note: changing the below configuration might increase the max allocation size for a vector
+ which in turn can cause OOM. -->
+ <argLine>-Darrow.vector.max_allocation_bytes=1048576</argLine>
Review comment:
It seems this is duplicate with arrow.vector.max_allocation_bytes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-656015125
Thanks again @liyafan82 and @BryanCutler have addressed your comments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r452076262
##########
File path: java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java
##########
@@ -109,7 +109,8 @@ static AllocationManagerType getDefaultAllocationManagerType() {
field.setAccessible(true);
return (AllocationManager.Factory) field.get(null);
} catch (Exception e) {
- throw new RuntimeException("Unable to instantiate Allocation Manager for " + clazzName, e);
+ throw new RuntimeException("No DefaultAllocationManager found on classpath. Can't allocate Arrow buffers." +
+ " Please consider adding arrow-memory-netty or arrow-memory-unsafe as a dependency.");
Review comment:
good idea @BryanCutler that is much cleaner. Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451792885
##########
File path: java/memory/memory-unsafe/pom.xml
##########
@@ -0,0 +1,54 @@
+<?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>1.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-memory-unsafe</artifactId>
+ <name>Arrow Memory - Unsafe</name>
+ <description>Allocator and utils for allocating memory in Arrow based on sun.misc.Unsafe</description>
+
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.arrow</groupId>
+ <artifactId>arrow-memory-core</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <version>3.0.0-M3</version>
+ <configuration>
+ <enableAssertions>true</enableAssertions>
+ <childDelegation>true</childDelegation>
+ <forkCount>${forkCount}</forkCount>
+ <reuseForks>true</reuseForks>
+ <systemPropertyVariables>
+ <java.io.tmpdir>${project.build.directory}</java.io.tmpdir>
+ <user.timezone>UTC</user.timezone>
+ </systemPropertyVariables>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
+</project>
Review comment:
It's usually conventional to have it at the end of text files and some command line tools expect it. Not sure why it's not caught by the style checker, but that's why github marks it with a red icon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#issuecomment-655849190
> Hey all,
>
> Thanks @BryanCutler and @liyafan82 for the comments and thanks @jacques-n for the reasoning behind unsafe/netty module split.
>
> I have addressed your comments in the most recent update and have created two new tickets:
>
> 1. bump netty version: https://issues.apache.org/jira/browse/ARROW-9370
> 2. run tests twice: https://issues.apache.org/jira/browse/ARROW-9371
>
> I will raise PRs for these soon
@rymurr Mostly look good to me. Thanks for your effort.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #7619: ARROW-9300: [Java] Separate Netty Memory to its own module
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #7619:
URL: https://github.com/apache/arrow/pull/7619#discussion_r451797963
##########
File path: java/memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java
##########
@@ -109,7 +109,8 @@ static AllocationManagerType getDefaultAllocationManagerType() {
field.setAccessible(true);
return (AllocationManager.Factory) field.get(null);
} catch (Exception e) {
- throw new RuntimeException("Unable to instantiate Allocation Manager for " + clazzName, e);
+ throw new RuntimeException("No DefaultAllocationManager found on classpath. Can't allocate Arrow buffers." +
+ " Please consider adding arrow-memory-netty or arrow-memory-unsafe as a dependency.");
Review comment:
Hmm, this one is a little different. It looks like it could be from `getUnsafeFactory()`, `getNettyFactory()`, or some custom class. I would leave the original exception with `clazzName`, then in `getUnsafeFactory()`, `getNettyFactory()` catch the RuntimeException and add a message like "Please ensure [arrow-memory-netty, arrow-memory-unsafe] or equivalent class containing the DefaultAllocationManager is on the classpath"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org