GH-3558: Properly close buffers#3559
Conversation
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
It would be good if @gszadovszky could take a look at this as this is related to TrackingByteBufferAllocator.
| <shade.prefix>shaded.parquet</shade.prefix> | ||
| <!-- Guarantees no newer classes/methods/constants are used by parquet. --> | ||
| <hadoop.version>3.3.0</hadoop.version> | ||
| <hadoop.version>3.4.0</hadoop.version> |
There was a problem hiding this comment.
Do we want to reflect this in the PR title?
There was a problem hiding this comment.
This is the underbound of the supported Hadoop version, so I think it is best to set this to 3.3.0 before merging this. Unless @steveloughran has an opinion on a reasonable lower-bound :)
| @Override | ||
| public void close() throws LeakedByteBufferException { | ||
| if (!allocated.isEmpty()) { | ||
| LeakedByteBufferException ex = new LeakedByteBufferException( |
There was a problem hiding this comment.
This looks like a behavior change?
There was a problem hiding this comment.
Yes, it is, but it is not covered in any tests. WDYT @gszadovszky ?
There was a problem hiding this comment.
I don't think this was working properly since Hadoop did raise exception around not properly closing buffers, while this exception wasn't being thrown. Let me dig a bit further here
There was a problem hiding this comment.
Yeah. The whole point of this allocator implementation is to fail if something was not released. It may make sense to actually release the buffers so the tests won't leak, be we need to throw that exception, otherwise this class is pointless.
There was a problem hiding this comment.
When I've created this class, I've caught a couple of leaks in parquet-java code with it. Not having that exception thrown during our unit tests means the leaks are fixed not that the monitoring class is not working :)
Fokko
force-pushed
the
fd-correctly-close-buffers
branch
3 times, most recently
from
eb75160 to
ba219c3
Compare
Fokko
force-pushed
the
fd-correctly-close-buffers
branch
from
ba219c3 to
d1a7cec
Compare
3 participants
Rationale for this change
With updating to a later version of Hadoop, we got the error that there are lingering buffers that are not closed properly. This PR addresses this issue by properly releasing the acquired buffers.
Initially caught by @pan3793 in #3360 (comment)
Closes #3558
Closes #3356
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?