ARROW-9371: [Java] Run vector tests for both allocators #7676
Conversation
java/vector/pom.xml
Outdated
There was a problem hiding this comment.
Can we simply remove this section?
There was a problem hiding this comment.
Hey @liyafan82, we can skip this section but we will run the tests 3 times then. One of the three will be with an unknown Allocator (it will be chosen at runtime depending on classpath). I thought it was a bit of a waste.
There was a problem hiding this comment.
Thanks for your clarification.
There was a problem hiding this comment.
When we run tests from IDE, it is also with an unknown allocator, right?
There was a problem hiding this comment.
yes, thats correct. We would have to set an env variable in the IDE to choose one or the other.
There was a problem hiding this comment.
would it work to remove the skip=true here and add <classpathDependencyExclude>org.apache.arrow:arrow-memory-unsafe</classpathDependencyExclude>, so that the default test runs netty, then leave the additional run-unsafe section only?
There was a problem hiding this comment.
yes, that is quite a bit simpler. Apologies for misunderstanding originally.
I have made that change
There was a problem hiding this comment.
I do not find the code for closing the allocator
There was a problem hiding this comment.
It would be nice to implement the code by try-with-resource
There was a problem hiding this comment.
thanks, agreed. Fixed!
rymurr
force-pushed
the
ARROW-9371
branch
2 times, most recently
from
e993dc0 to
7cf3ecc
Compare
|
Build fail could be a random network error or related to the parallel build. Have rebased & retriggered. |
BryanCutler
left a comment
BryanCutler
left a comment
There was a problem hiding this comment.
LGTM, just a couple minor things
java/vector/pom.xml
Outdated
There was a problem hiding this comment.
I think you can remove the comment now
java/vector/pom.xml
Outdated
There was a problem hiding this comment.
rename to run-unsafe please
Thanks for the review @BryanCutler I won't rush a small change next time and save you the trouble ;-) |
|
Did a quick timing test for this, as expected it doubles the test time for arrow-vector, which is ~10s on my laptop. That seems ok to me. Before After |
|
@rymurr looks like this needs a rebase |
As per apache#7619 (comment) the vector tests should be run for both netty and unsafe allocators
|
rebase is done, thanks @BryanCutler I agree re timing, I saw the same numbers. On my laptop the vector compilation time dwarfs the test time so doubling the test time didn't seem too bad. |
|
merged to master, thanks @rymurr ! |
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>
3 participants
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-unsafetests are done with the Unsafe Allocator.