HBASE-28390 WAL value compression fails for cells with large values #5696
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
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
apurtell
reviewed
Feb 22, 2024
| } | ||
| } | ||
|
|
||
| private int rawReadInt(InputStream in) throws IOException { |
There was a problem hiding this comment.
I'm surprised commons-io or our own util code doesn't have something like this already.
There was a problem hiding this comment.
I tried looking and couldn't find one. I'll try looking again
There was a problem hiding this comment.
Woops I think IOUtils.readInt should work. Any other comments/concerns other than that?
There was a problem hiding this comment.
Left an approving review with the rest. Thanks!
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
| @@ -67,14 +71,36 @@ public class CompressedWALTestBase { | |||
| Arrays.fill(VALUE, off, (off += 1597), (byte) 'Q'); | |||
| } | |||
|
|
|||
| public void doTest(TableName tableName) throws Exception { | |||
| @Test | |||
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) { |
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>
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.
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.