Uh oh!
There was an error while loading. Please reload this page.
HBASE-28390 WAL value compression fails for cells with large values - #5696
Merged
Conversation
Apache-HBase
commented
Feb 22, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 22, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 22, 2024
🎊 +1 overall
This message was automatically generated. |
| } | ||
| } | ||
| private int rawReadInt(InputStream in) throws IOException { |
Contributor
There was a problem hiding this comment.
I'm surprised commons-io or our own util code doesn't have something like this already.
ContributorAuthor
There was a problem hiding this comment.
I tried looking and couldn't find one. I'll try looking again
ContributorAuthor
There was a problem hiding this comment.
Woops I think IOUtils.readInt should work. Any other comments/concerns other than that?
Contributor
There was a problem hiding this comment.
Left an approving review with the rest. Thanks!
ContributorAuthor
There was a problem hiding this comment.
Actually, there is no IOUtils.readInt in commons-io. I realized that the IOUtils I was referring to (while on mobile) was a different library that we don't have a dependency on.
We have a variety of methods for reading ints from byte[] or ByteBuffer, but not for InputStream that I can find. I could read the bytes into a byte[4] and use one of the utils, but I'd rather not allocate an unnecessary byte array. I'm going to leave this alone for now.
apurtell
approved these changes
Feb 22, 2024
| } | ||
| public void doTest(TableName tableName) throws Exception { | ||
| @Test |
Contributor
There was a problem hiding this comment.
Thanks for cleaning this up and removing all the unneeded boilerplate.
| // if the uncompressed size was larger than the configured buffer size for the codec, | ||
| // the BlockCompressorStream will have left an extra 4 bytes hanging. This represents a size | ||
| // for the next segment, and it should be 0. See HBASE-28390 | ||
| if (lowerIn.available() == 4) { |
Contributor
There was a problem hiding this comment.
This part is ugly but I don't have an alternative suggestion. At least we know both in the WAL read and HFile read cases we won't be contending with a short read, so available is going to be either 0 or 4 and otherwise we'd have had an EOFException thrown.
bbeaudreault added a commit
that referenced
this pull request
Feb 23, 2024
…5696) Signed-off-by: Andrew Purtell <apurtell@apache.org>
bbeaudreault added a commit
that referenced
this pull request
Feb 23, 2024
…5696) Signed-off-by: Andrew Purtell <apurtell@apache.org>
bbeaudreault added a commit
that referenced
this pull request
Feb 23, 2024
…5696) Signed-off-by: Andrew Purtell <apurtell@apache.org>
bbeaudreault added a commit
that referenced
this pull request
Feb 23, 2024
…5696) Signed-off-by: Andrew Purtell <apurtell@apache.org>
bbeaudreault added a commit
to HubSpot/hbase
that referenced
this pull request
Mar 4, 2024
…ith large values (apache#5696) Signed-off-by: Andrew Purtell <apurtell@apache.org>
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Unify all of the WAL compression tests to utilize tests defined in the base class. Add a
testLargetest which writes large buffers. Prior to the fixes in CompressionContext and WALDecompressionBoundedDelegatingInputStream, these tests all failed.