-
Notifications
You must be signed in to change notification settings - Fork 1.5k
PARQUET-1320: Fix potential direct memory leak for snappy decompressor #492
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
|
Can one of the owner help review this?Thanks |
|
@caneGuy, can you fix the test failures? Also, can you more clearly describe the situation this is intended to fix and how the fix works? |
|
|
||
| private boolean finished; | ||
|
|
||
| private int maxBufferSize = 64 * 1024 * 1024; |
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.
Why was this introduced?
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.
Used for reset below.We can remove this.But i think for the purpose of this patch,we should also refactor reset api.
Any suggestion?Thanks @rdblue
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.
Can you elaborate more than "used for reset below"? What was the intent? Also, why do you think the reset API needs to be refactored?
|
Sorry, @rdblue since i can't find the actual failure test case. |
|
I think the failure was caused by |
|
@caneGuy Can you trigger a retest? |
|
Sorry @wangyum i don't know how to retest.Could you help trigger? |
|
If you are trying to build a pull-request, you can mark it closed and then mark it opened. This will trigger a new build. |
|
Thanks @wangyum done! |
|
I was having an issue with a new service we're building that processes a bunch of snappy encoded parquet files OOM'ing. This patch seemed to resolve the issue for us. I also played around with having the cleaner only run on reset, didn't seem to resolve the issue. |
|
May be i can refactor later @nputnam |
When use NonBlockedDecompressorStream for decompress.
If we do not get any full gc for old gen.we may failed by off-heap memory leak
This patch fix this potential leak.