ARROW-8230: [Java] Remove netty dependency from arrow-memory #7347
Conversation
There was a problem hiding this comment.
we should print some logs here?
There was a problem hiding this comment.
sure, any suggestions where? Note: This is taken from PlatformDependent0 in netty (logs and all).
There was a problem hiding this comment.
looks good. thanks.
since the exception handling logic is identical, does it make the code conciser to use the following semantics?
catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
...
}
There was a problem hiding this comment.
It would be helpful for problem diagnostic, if we could print some logs here?
There was a problem hiding this comment.
Is it possible that this value changes from Netty code base?
There was a problem hiding this comment.
It is possible that it changes. It was a bit involved to copy the Netty code to get chunk size into the arrow codebase so I left it at a likely value for x86-64 machines. I am happy to replace this with the Netty value or have chunk size taken from the actual allocator directly if you thikn that is better than a single static value?
There was a problem hiding this comment.
Maybe it is beneficial to add above discussion to the JavaDoc
There was a problem hiding this comment.
I ended up taking the implementation from Netty rather than the static value here. I thought it was a bit safer.
rymurr
force-pushed
the
ARROW-8230
branch
2 times, most recently
from
3585eb6 to
7b2a9e1
Compare
|
Thanks for the reviews @liyafan82 I have updated based on your comments. Please let me know what you think! |
|
@rymurr Thanks for your effort. I will make another pass today. |
There was a problem hiding this comment.
please remove "DEFAULT_PAGE_SIZE" here
There was a problem hiding this comment.
Please remove "DEFAULT_MAX_ORDER" here
There was a problem hiding this comment.
Also here we need only one parameter in the "else" statement.
There was a problem hiding this comment.
here the log level should be warning or error?
There was a problem hiding this comment.
Is there a special reason for the cast here?
There was a problem hiding this comment.
None, only that it was how the netty developers wrote it. I thought I would copy their lead. However, I have simplified it now,
|
Mostly looks good to me. There are a few minor issues. |
|
Thanks @liyafan82 I have updated based on your comments and the integration tests have passed. |
|
looks like an ongoing github incident is causing build failures. Will rebase and rebuild once Github is back to normal |
There was a problem hiding this comment.
I think here we only need one parameter here? So it should be
logger.debug("-Dorg.apache.memory.allocator.pageSize: {}", pageSizeFallbackCause);
|
@rymurr Thanks for your work. A few typos. |
|
Thanks a lot @liyafan82 I have addressed your suggestions and rebased |
This commit moves all Netty specific calls into a few classes. This is the precursor to splitting the netty and unsafe allocators out to their own modules
@rymurr Thanks for your work. Will merge when it turns green. |
|
@liyafan82 if you aren't already please make sure you use the merge script under dev to merge PRs |
@emkornfield Thanks a lot for your kind reminder. I will use the script next time. |
…7347) This commit moves all Netty specific calls into a few classes. This is the precursor to splitting the netty and unsafe allocators out to their own modules
3 participants
This commit moves all Netty specific calls into a few classes.
This is the precursor to splitting the netty and unsafe allocatorsout to their own modules