lidavidm commented on PR #13248: URL: https://github.com/apache/arrow/pull/13248#issuecomment-1260053398
My objection here is that the semantics of what is going on here are unclear. We can add code that makes things pass tests but if the _intent_ is already hard to follow then we're just setting up future library users - and ourselves - for more confusion. In any case I don't have enough time really to look into things deeply. So if another maintainer wants to merge this then I'm not going to object. I still think we're too fixated on this being a memory 'allocator' when it seems more like a 'nursery' object. Also I guess going back to this > I guess this is reasonable given where we are, but associating with an allocator that isn't doing the allocation seems likely to confuse future developers if the memory appears to be managed in the Java code but isn't. Is the idea that all users of a given bit of memory keep track of their own use, but only the originator (C++ presumably in this case) actually frees it? This makes sense to me from the 'nursery' or 'accountant' perspective. Closing a buffer delegates to the specific ReferenceManager implementation, which may use Java code or native code. It also updates the allocation metric in the BufferAllocator, which is separate from how the buffer is actually implemented. Then the application closes the BufferAllocator, which can check at runtime that the application actually closed all buffers it claims were associated with this scope. Transfer, in this scheme, is a way to change what object is responsible for allocated bytes, as opposed to allocating new ones or freeing allocated ones. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
