ARROW-9300: [Java] Separate Netty Memory to its own module #7619

rymurr · 2020-07-02T16:32:16Z

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

github-actions · 2020-07-02T16:46:37Z

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

liyafan82 · 2020-07-03T09:50:20Z

...emory/memory-core/src/test/java/org/apache/arrow/memory/DefaultAllocationManagerFactory.java

Since this is a frequently called method, I think it would be beneficial to reuse the empty ArrowBuf.

This is only in tests, but I agree. Fixed

liyafan82 · 2020-07-03T09:54:44Z

java/vector/pom.xml

Maybe we also need the memory-unsafe dependency here?
Otherwise, our integeration tests will not be able to pass.

I don't think the integration tests use UnsafeAllocator do they?

I ran mvn clean install -Pintegration-tests and it was clean. So the integration tests went ok w/ Netty

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.

I believe the netty allocator can allocate 64-bit spaces but netty buffers can't directly reference them?

Maybe we should run the unit tests once for each of the 2 allocators?

rymurr · 2020-07-03T12:01:17Z

Thanks for the comments @liyafan82 . I have updated and rebased to pull in your fix from #7628

liyafan82 · 2020-07-03T12:17:50Z

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.

liyafan82 · 2020-07-06T07:47:03Z

...emory/memory-core/src/test/java/org/apache/arrow/memory/DefaultAllocationManagerFactory.java

We have 3 classes in the code base named "DefaultAllocationManagerFactory". Do we need all of them?

Hey @liyafan82, yes i think we do:

lives in the netty module.

lives in the unsafe module

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.

BryanCutler

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 · 2020-07-07T19:47:09Z

java/adapter/orc/pom.xml

was this intentional?

no, don't know how that snuck through. Fixed

BryanCutler · 2020-07-07T20:27:44Z

java/vector/pom.xml

This could be removed now right?

BryanCutler · 2020-07-07T20:59:36Z

java/memory/memory-netty/pom.xml

add newline?

it still looks like it needs a newline at the end of the file

BryanCutler · 2020-07-07T21:01:16Z

java/vector/pom.xml

Maybe we should run the unit tests once for each of the 2 allocators?

BryanCutler · 2020-07-07T21:04:46Z

java/memory/memory-core/pom.xml

is this necessary here?

jacques-n · 2020-07-08T00:33:03Z

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.

liyafan82 · 2020-07-08T01:46:37Z

java/memory/memory-core/pom.xml

This seems duplicate with the system property defined above

liyafan82 · 2020-07-08T01:51:07Z

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.

BryanCutler · 2020-07-08T02:33:08Z

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"?

BryanCutler · 2020-07-08T02:38:27Z

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

rymurr · 2020-07-08T09:23:13Z

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:

bump netty version: https://issues.apache.org/jira/browse/ARROW-9370
run tests twice: https://issues.apache.org/jira/browse/ARROW-9371

I will raise PRs for these soon

As per apache#7619 (comment) there is a security vulnerability in the current version of Netty. This upgrades to the latest version. A compatible upgrade of grpc was also required

BryanCutler

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)? Otherwise LGTM

BryanCutler · 2020-07-08T17:28:12Z

java/memory/memory-netty/pom.xml

it still looks like it needs a newline at the end of the file

BryanCutler · 2020-07-08T17:28:23Z

java/memory/memory-unsafe/pom.xml

newline here

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

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.

BryanCutler · 2020-07-08T17:28:45Z

java/memory/memory-core/pom.xml

newline here

I have added a new line in all 3 locations

rymurr · 2020-07-08T19:41:18Z

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)? Otherwise LGTM

Both are done now. I mistakenly missed the DefaultAllocationManagerFactory update in the previous change set

BryanCutler · 2020-07-08T20:10:57Z

...memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java

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"

good idea @BryanCutler that is much cleaner. Fixed

liyafan82 · 2020-07-09T01:44:48Z

...emory/memory-core/src/test/java/org/apache/arrow/memory/DefaultAllocationManagerFactory.java

I think this comment can be removed.

liyafan82 · 2020-07-09T01:48:01Z

java/memory/memory-netty/pom.xml

It seems this is duplicate with arrow.vector.max_allocation_bytes

agreed, removed

liyafan82 · 2020-07-09T01:52:56Z

java/performance/pom.xml

We also need the dependency of arrow-memory-netty here?

liyafan82 · 2020-07-09T01:55:27Z

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:

bump netty version: https://issues.apache.org/jira/browse/ARROW-9370

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.

As per #7619 (comment) there is a security vulnerability in the current version of Netty. This upgrades to the latest version. A compatible upgrade of grpc was also required Closes #7677 from rymurr/ARROW-9370 Authored-by: Ryan Murray <rymurr@dremio.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>

rymurr · 2020-07-09T09:20:13Z

Thanks again @liyafan82 and @BryanCutler have addressed your comments.

liyafan82

LGTM

BryanCutler

Thanks for the update, just need to fix the typo in the error message and I think this will be good.

BryanCutler · 2020-07-09T16:26:35Z

...memory/memory-core/src/main/java/org/apache/arrow/memory/DefaultAllocationManagerOption.java

should be arrow-memory-netty and NettyAllocationManager below

sigh....sorry. Copy paste for the win. Fixed.

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

BryanCutler

LGTM

BryanCutler · 2020-07-09T18:58:11Z

merged to master, thanks @rymurr !

As per apache#7619 (comment) the vector tests should be run for both netty and unsafe allocators

As per #7619 (comment) the vector tests should be run for both netty and unsafe allocators. The default tests are run with the Netty allocator, and `run-unsafe` tests are done with the Unsafe Allocator. Closes #7676 from rymurr/ARROW-9371 Authored-by: Ryan Murray <rymurr@dremio.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>

As per apache/arrow#7619 (comment) there is a security vulnerability in the current version of Netty. This upgrades to the latest version. A compatible upgrade of grpc was also required Closes #7677 from rymurr/ARROW-9370 Authored-by: Ryan Murray <rymurr@dremio.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>

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 Closes apache#7619 from rymurr/ARROW-9300 Authored-by: Ryan Murray <rymurr@dremio.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>

As per apache#7619 (comment) the vector tests should be run for both netty and unsafe allocators. The default tests are run with the Netty allocator, and `run-unsafe` tests are done with the Unsafe Allocator. Closes apache#7676 from rymurr/ARROW-9371 Authored-by: Ryan Murray <rymurr@dremio.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>

rymurr force-pushed the ARROW-9300 branch 2 times, most recently from d8a34cc to 6ea28a3 Compare July 2, 2020 16:43

rymurr force-pushed the ARROW-9300 branch 3 times, most recently from faa517f to 7dbb59f Compare July 3, 2020 08:54

liyafan82 mentioned this pull request Jul 3, 2020

ARROW-9315: [Java] Fix the failure of testAllocationManagerType #7628

Closed

rymurr force-pushed the ARROW-9300 branch from 7dbb59f to bd02647 Compare July 3, 2020 09:45

liyafan82 reviewed Jul 3, 2020

View reviewed changes

rymurr force-pushed the ARROW-9300 branch from bd02647 to fc58f1f Compare July 3, 2020 12:00

liyafan82 reviewed Jul 6, 2020

View reviewed changes

BryanCutler reviewed Jul 7, 2020

View reviewed changes

liyafan82 reviewed Jul 8, 2020

View reviewed changes

rymurr force-pushed the ARROW-9300 branch from fc58f1f to 850d3c1 Compare July 8, 2020 09:30

rymurr mentioned this pull request Jul 8, 2020

ARROW-9371: [Java] Run vector tests for both allocators #7676

Closed

rymurr mentioned this pull request Jul 8, 2020

ARROW-9370: [Java] Bump Netty version #7677

Closed

BryanCutler reviewed Jul 8, 2020

View reviewed changes

rymurr force-pushed the ARROW-9300 branch 2 times, most recently from 5dff1f8 to 928d971 Compare July 8, 2020 18:32

BryanCutler reviewed Jul 8, 2020

View reviewed changes

liyafan82 reviewed Jul 9, 2020

View reviewed changes

java/performance/pom.xml Outdated

Copy link

Contributor

liyafan82 Jul 9, 2020

Choose a reason for hiding this comment

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

We also need the dependency of arrow-memory-netty here?

rymurr force-pushed the ARROW-9300 branch from 928d971 to e158824 Compare July 9, 2020 09:18

liyafan82 approved these changes Jul 9, 2020

View reviewed changes

BryanCutler requested changes Jul 9, 2020

View reviewed changes

rymurr force-pushed the ARROW-9300 branch from e158824 to 9b12e89 Compare July 9, 2020 16:52

BryanCutler approved these changes Jul 9, 2020

View reviewed changes

BryanCutler closed this in 2092e18 Jul 9, 2020

rymurr deleted the ARROW-9300 branch July 22, 2020 11:58

rymurr pushed a commit to rymurr/arrow that referenced this pull request Jul 24, 2020

ARROW-9371: [Java] Run vector tests for both allocators

ed76e77

As per apache#7619 (comment) the vector tests should be run for both netty and unsafe allocators

This was referenced Oct 1, 2020

[Java] Separate Netty Memory to its own module #25389

Closed

[Java] Bump Netty version #25453

Closed

[Java] Run vector tests for both allocators #25454

Closed

ARROW-9300: [Java] Separate Netty Memory to its own module #7619

ARROW-9300: [Java] Separate Netty Memory to its own module #7619

Uh oh!

Conversation

rymurr commented Jul 2, 2020

Uh oh!

github-actions bot commented Jul 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rymurr commented Jul 3, 2020

Uh oh!

liyafan82 commented Jul 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacques-n commented Jul 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyafan82 commented Jul 8, 2020

Uh oh!

BryanCutler commented Jul 8, 2020

Uh oh!

BryanCutler commented Jul 8, 2020

Uh oh!

rymurr commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rymurr commented Jul 8, 2020 •

edited

Loading