HBASE-27053 IOException during caching of uncompressed block to the block cache #4610
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
HBASE-27053
branch
4 times, most recently
from
d1f67b2
to
2531a95
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
HBASE-27053
branch
from
d8ce9bb
to
02c85ab
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@apurtell this is ready for review now. The last build was successful, I just pushed a small cleanup. @Apache9 you may be interested in this. I know you aren't super familiar with the BucketCache, but this is mostly just related to how we read HFileBlocks from disks. I think there is another potential solution here which is likely a lot simpler -- Similar to #4453, modify HFileBlockDefaultDecodingContext.prepareDecoding to always fill out the checksum bytes. In that PR he fills out the bytes with zero's. It seems like if we're going to leave space for the checksum, we should instead just do another That said, this solution is more cpu and space efficient, because we aren't leaving space for or copying over a checksum that we will never use. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Jul 16, 2022
| @@ -1709,6 +1700,9 @@ protected HFileBlock readBlockDataInternal(FSDataInputStream is, long offset, | |||
| if (verifyChecksum && !validateChecksum(offset, curBlock, hdrSize)) { | |||
| return null; | |||
| } | |||
| // remove checksum from buffer now that it's verified | |||
| int sizeWithoutChecksum = curBlock.getInt(Header.ON_DISK_DATA_SIZE_WITH_HEADER_INDEX); | |||
| curBlock = curBlock.duplicate().limit(sizeWithoutChecksum); | |||
There was a problem hiding this comment.
Do we need to duplicate here? The old instance could be reference by others?
There was a problem hiding this comment.
Thanks for reviewing and good catch. I just pushed a fix for that. I also added a few more assertions to my new test
wenwj0
approved these changes
Jul 16, 2022
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Jul 16, 2022
| @@ -1709,6 +1700,9 @@ protected HFileBlock readBlockDataInternal(FSDataInputStream is, long offset, | |||
| if (verifyChecksum && !validateChecksum(offset, curBlock, hdrSize)) { | |||
| return null; | |||
| } | |||
| // remove checksum from buffer now that it's verified | |||
| int sizeWithoutChecksum = curBlock.getInt(Header.ON_DISK_DATA_SIZE_WITH_HEADER_INDEX); | |||
| curBlock = curBlock.limit(sizeWithoutChecksum); | |||
There was a problem hiding this comment.
This is just a no op? curBlock.limit just returns curBlock itself?
There was a problem hiding this comment.
ok. I personally prefer to use the returned value in cases like this, in case the implementation changes. but i'm happy to not do that if it's more generally preferred. i just pushed a change to remove that.
Apache9
approved these changes
Jul 16, 2022
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Test looks unrelated, but I ran it locally anyway and it looks good. |
bbeaudreault
added a commit
that referenced
this pull request
Jul 16, 2022
…lock cache (#4610) Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: wenwj0 <wenweijian2@huawei.com>
bbeaudreault
added a commit
that referenced
this pull request
Jul 16, 2022
…lock cache (#4610) Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: wenwj0 <wenweijian2@huawei.com>
bbeaudreault
added a commit
that referenced
this pull request
Jul 16, 2022
…lock cache (#4610) Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: wenwj0 <wenweijian2@huawei.com>
bbeaudreault
added a commit
to HubSpot/hbase
that referenced
this pull request
Jul 17, 2022
…sed block to the block cache (apache#4610) Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: wenwj0 <wenweijian2@huawei.com>
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…lock cache (apache#4610) Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: wenwj0 <wenweijian2@huawei.com> (cherry picked from commit 33b25ab) Change-Id: Ibfb9c84979d19b765baae48fd66e17a46de2bcd5
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Tests a different fix for HBASE-27053, based on analysis I posted in the issue.