-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-9475: [Java] Clean up usages of BaseAllocator, use BufferAllocator in… #7768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for opening a pull request! Could you open an issue for this pull request on JIRA? Then could you also rename pull request title in the following format? See also: |
|
This patch is also supposed to be a dependency of #7030 (ARROW-7808). |
rymurr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, one comment about the allocate/release additions. Otherwise its great to further limit BaseAllocators visibility
| * @param size to increase | ||
| * @return Whether the allocation fit within limits. | ||
| */ | ||
| boolean forceAllocate(long size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see where this or releaseBytes are used in code. What is the purpose of releaseBytes and of makign forceAllocate public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rymurr,
E.g. Please see the changes such as
The uses of BufferAllocator requires for having the relevant methods extracted up to interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 sorry, missed that
|
@jacques-n Could you please talk a look? Thanks. For ARROW-7808, having this one solved we could be able to leave customized AllocationManager implementation out of the package |
|
+1 |
…tor in… …stead Issue link: https://issues.apache.org/jira/browse/ARROW-9475. Closes #7768 from zhztheplayer/ARROW-9475 Authored-by: Hongze Zhang <[email protected]> Signed-off-by: Micah Kornfield <[email protected]>
…tor in… …stead Issue link: https://issues.apache.org/jira/browse/ARROW-9475. Closes apache#7768 from zhztheplayer/ARROW-9475 Authored-by: Hongze Zhang <[email protected]> Signed-off-by: Micah Kornfield <[email protected]>
…stead
Issue link: https://issues.apache.org/jira/browse/ARROW-9475.