HBASE-29857 better handling NPE in BucketCache #7685
+15
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
vaijosh
suggested changes
Jan 29, 2026
|
|
||
| // HBASE-29857: Handle case where persistence file is empty or corrupted. | ||
| // parseDelimitedFrom() returns null when there's no data to read. | ||
| if (cacheEntry == null) { |
Contributor
There was a problem hiding this comment.
@teamconfx
I noticed one more potential NPE in BucketCache.retrieveFromFile e.g. when ProtobufMagic.isPBMagicPrefix(pbuf)=true.
So can we validation check in the caller, BucketCache.retrieveFromFile to check if the persistence file is empty or corrupted?
Contributor
Author
There was a problem hiding this comment.
Thanks for pointing this out! Will do it.
Contributor
Author
There was a problem hiding this comment.
@vaijosh I added a similar validation check for BucketCache.retrieveFromFile, can you please take another look? Thanks!
Contributor
There was a problem hiding this comment.
We should re-phrase the comment, as this only handles the case for empty files. Corrupt files would likely cause a InvalidProtocolBufferException, which is a sub-class of IOException.
This comment has been minimized.
This comment has been minimized.
vaijosh
approved these changes
Jan 29, 2026
Contributor
|
Thanks @teamconfx |
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
reviewed
Jan 29, 2026
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
Have a few minor comments. Apart from those, can we add UTs for these conditions?
Comment on lines
1615
to
1621
| // HBASE-29857: Validate that the persistence file has data after the magic bytes. | ||
| // A truncated or corrupted file may only contain magic bytes without actual cache data. | ||
| if (in.available() == 0) { | ||
| throw new IOException("Persistence file appears to be truncated or corrupted. " | ||
| + "File contains only magic bytes without cache data: " + persistencePath); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Do we really need this extra check? I guess if there's nothing after pb magic in the stream, we will reach the condition where BucketCacheProtos.BucketCacheEntry.parseDelimitedFrom(in) returns null, which is already been validated for both cases.
Contributor
Author
There was a problem hiding this comment.
True, I can remove this extra check.
|
|
||
| // HBASE-29857: Handle case where persistence file is empty or corrupted. | ||
| // parseDelimitedFrom() returns null when there's no data to read. | ||
| if (cacheEntry == null) { |
Contributor
There was a problem hiding this comment.
We should re-phrase the comment, as this only handles the case for empty files. Corrupt files would likely cause a InvalidProtocolBufferException, which is a sub-class of IOException.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is for JIRA issue: https://issues.apache.org/jira/browse/HBASE-29857
The original JIRA issue is found in HBase v2.6.3, and fixed by HBASE-28839.
I tested with the same workload that I see the bug from v2.6.3 and verify the fix works in both master and branch-2.6.
However the current code still not handle NPE when persistence file is empty or corrupted explicitly.
This PR proposes a new checker to better catch the null from
BucketCacheProtos.BucketCacheEntry.parseDelimitedFrom(in);.