DRILL-3811: AtomicRemainder incorrectly accounts for transferred allo…

[adeneche]

…cations

[cwestin]

I was surprised this compiled; in one of my recent patches, I had removed the "public" from TopLevelAllocator, but it seems to have come back. It was removed to force people to use RootAllocatorFactory.newRoot() instead of using the TopLevelAllocator constructor directly. Can you please remove the public from TopLevelAllocator again, and replace this constructor here and on the next line with RootAllocatorFactory.newRoot()?

[adeneche]

sure

[cwestin]

Just one comment, otherwise +1 (non-binding).

[adeneche]

TopLevelAllocator's constructor is not public but because the unit test is in the same package it was able to access the constructor anyway. Fixed the test to use RootAllocatorFactory instead

[amansinha100]

Since this will be subtracting size bytes, should there be a check for availableShared >=0 ? I am not quite sure what's supposed to happen if this value drops below 0 at this point...

[adeneche]

It can become negative if the allocator takes ownership of a buffer and exceeds it's maximum allocated memory. Negative values are handled properly, at least that's my understanding

[amansinha100]

It is likely handled..I will defer to Chris on that.

[cwestin]

This call is used when transferring ownership from the RPC layer to a receiving fragment -- and you can't refuse it -- the RPC layer has to hand it off. The problem is that the accounting for buffer ownership transfer and sharing is questionable in the original allocator. This is probably the best that can be done for now.

[amansinha100]

+1 .