Uh oh!
There was an error while loading. Please reload this page.
HBASE-27264 Add options to consider compressed size when delimiting blocks during hfile writes - #4675
Merged
Merged
Conversation
…locks during hfile writes
wchevreuil
requested review from
Apache9, ankitsinghal, apurtell, bbeaudreault, bharathv and saintstackAugust 2, 2022 11:54
Apache-HBase
commented
Aug 2, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 8, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 8, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 8, 2022
🎊 +1 overall
This message was automatically generated. |
| finishBlock(); | ||
| } | ||
| public boolean shouldFinishBlock() throws IOException { |
Contributor
There was a problem hiding this comment.
is there a reason to have this logic here vs in HFileWriteRImpl with the rest of the shouldfinish logic?
ContributorAuthor
There was a problem hiding this comment.
This method involves dealing with some block specifics, like compression, the block content byte array buffer and what to do with compression size when deciding what should be a block limit. Moving it to HFileWriteRImpl would spill some block specific variables and logic into the file writer logic. It just feels to me, putting it here is more cohesive.
| shouldFinishBlock = blockWriter.encodedBlockSizeWritten() >= hFileContext.getBlocksize() | ||
| || blockWriter.blockSizeWritten() >= hFileContext.getBlocksize(); | ||
| } | ||
| if (blockWriter.isSizeLimitCompressed()) { |
Contributor
There was a problem hiding this comment.
let's change this to if (blockWriter.isSizeLimitCompressed() && !shouldFinishBlock)? Just noting your comment on the calculation of compression ratio in the other file, we could further avoid that cost if we already know we need to finish the block for other reasons.
ContributorAuthor
There was a problem hiding this comment.
We actually want to enter here if shouldFinishBlock is true. Because that means the raw/encoded uncompressed size is larger than BLOCK_SIZE. But we don't know if the compressed size is smaller than BLOCK_SIZE, so we'll call blockWriter.shouldFinishBlock() to find that out.
And in the case where shouldFinishBlock is false, we'll not actually calculate the compressed size inside blockWriter.shouldFinishBlock(), because we don't go beyond this point.
taklwu
reviewed
Aug 9, 2022
taklwu
left a comment
Contributor
There was a problem hiding this comment.
mostly LGTM, just one question about the additional blockWriter.shouldFinishBlock() with original shouldFinishBlock logic.
Uh oh!
There was an error while loading. Please reload this page.
| || blockWriter.blockSizeWritten() >= hFileContext.getBlocksize(); | ||
| } | ||
| if (blockWriter.isSizeLimitCompressed()) { | ||
| shouldFinishBlock &= blockWriter.shouldFinishBlock(); |
Contributor
There was a problem hiding this comment.
related to the above comment by @bbeaudreault
in what situation the shouldFinishBlock = true and blockWriter.shouldFinishBlock() = false ? is it possible?
ContributorAuthor
There was a problem hiding this comment.
Yes, shouldFinishBlock could be true at this point because so far here we just checked "raw" uncompressed size or encoded uncompressed against BLOCK_SIZE. It is possible that these sizes are higher than BLOCK_SIZE, but the compressed size might still be less than the BLOCK_SIZE.
Apache9
commented
Aug 9, 2022
Contributor
I think this is a useful feature. When implementing an in-house LSM tree based storage system, I used to make use of the compression rates for the already written blocks to predicate the compression rate of the next block, to determine whether we should finish a block. I think the approach here is also OK, compressing once when we reach the default block size to predicate the compression rate. Maybe we could introduce something like a plugin to predicate the compression rate? The default implementation just always returns 1, and we could introduce different algorithms to predicate the compression rate of the current block size. What do you guys think? For me, the option names are a bit confusing... What does 'size.limit.compressed' mean? Thanks. |
Apache-HBase
commented
Aug 9, 2022
🎊 +1 overall
This message was automatically generated. |
wchevreuil
commented
Aug 9, 2022
ContributorAuthor
I thought about it originally, but then concluded this feature made more sense as an on/off (off by default) behaviour. Do you see other variations in between?
The properties were a bit confusing indeed. I tried to rename those now, let me know if those are more intuitive. |
wchevreuil
commented
Aug 9, 2022
ContributorAuthor
Sorry, I think I misunderstood it, previously. What if we leave this current PR as is, for an initial approach on using compressed sizes to determine blocks, then work on the pluggable solution on a separate ticket? |
Apache9
commented
Aug 11, 2022
Contributor
If we expose the configs out then it will be a pain to switch to a pluggable solution because we can not simply remove these configs without a deprecation cycle... I still prefer we make a pluggable solution first, and then the first approach could be the algorithm in this PR. |
wchevreuil
commented
Aug 11, 2022
ContributorAuthor
Ok, had just pushed a new commit where the algorithm for defining the compression rate and adjust the block size limit is pluggable. |
Apache-HBase
commented
Aug 11, 2022
💔 -1 overall
This message was automatically generated. |
ankitsinghal
requested changes
Aug 11, 2022
| static final byte[] DUMMY_HEADER_NO_CHECKSUM = | ||
| new byte[HConstants.HFILEBLOCK_HEADER_SIZE_NO_CHECKSUM]; | ||
| public static final String MAX_BLOCK_SIZE_UNCOMPRESSED = "hbase.block.max.size.uncompressed"; |
Contributor
There was a problem hiding this comment.
Can we move this and it's logic to the predicate itself?
| // In order to avoid excessive compression size calculations, we do it only once when | ||
| // the uncompressed size has reached BLOCKSIZE. We then use this compression size to | ||
| // calculate the compression rate, and adjust the block size limit by this ratio. | ||
| if (adjustedBlockSize == 0 || uncompressedBlockSize >= adjustedBlockSize) { |
Contributor
There was a problem hiding this comment.
We are recomputing compression size every time if we are even a little over the previously calculated adjusted block size, which is possible for all the blocks and will result in computation all the time?
ContributorAuthor
There was a problem hiding this comment.
Well, whenever we are closing the current block, based on adjusted block size value from previous block. It doesn't add much overhead because we are already closing the block, regardless. Main problem here, is that we don't know yet if it's little over or not, because we have been checking only uncompressed size.
Comment on lines
+55
to
+59
| int compressedSize = EncodedDataBlock.getCompressedSize(context.getCompression(), | ||
| context.getCompression().getCompressor(), contents.getBuffer(), 0, | ||
| contents.size()); | ||
| adjustedBlockSize = uncompressedBlockSize / compressedSize; | ||
| adjustedBlockSize *= context.getBlocksize(); |
Contributor
There was a problem hiding this comment.
I was expecting the algorithm to be
- Let the first block get closed with the regular adjustedBlockSize == block_size
- Have a method in this predicate to be called on finishBlock and use the currently closed block statistics to determine the compression ratio and store the adjusted size.
- Use the above compression ratio/adjusted size to determine the boundaries of the next block.
This way, we will keep on adjusting the ratio for the next block as per the previous block without doing any extra compression
ContributorAuthor
There was a problem hiding this comment.
Had pushed a new commit with this approach.
ankitsinghal
left a comment
Contributor
There was a problem hiding this comment.
Few comments on the current alogrithm.
Apache-HBase
commented
Aug 12, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 12, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 13, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 13, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 13, 2022
💔 -1 overall
This message was automatically generated. |
ankitsinghal
approved these changes
Aug 13, 2022
ankitsinghal
left a comment
Contributor
There was a problem hiding this comment.
I have left a few comments that you can complete before committing. +1 so that you don't need to wait for me to commit it.
Comment on lines
+52
to
+54
| if (uncompressedBlockSize >= adjustedBlockSize) { | ||
| adjustedBlockSize = context.getBlocksize() * compressionRatio; | ||
| } |
Contributor
There was a problem hiding this comment.
I'll remove the (uncompressedBlockSize >= adjustedBlockSize) check so that we are adjusting size on the basis of previous block compression everytime by calculating the adjustedBlockSize in updateLatestBlockSizes itself, And from this method only return it
Comment on lines
+914
to
+927
| public boolean shouldFinishBlock() throws IOException { | ||
| // int uncompressedBlockSize = blockSizeWritten(); | ||
| int uncompressedBlockSize = baosInMemory.size(); | ||
| if (uncompressedBlockSize >= fileContext.getBlocksize()) { | ||
| if (uncompressedBlockSize < maxSizeUnCompressed) { | ||
| int adjustedBlockSize = compressedSizePredicator. | ||
| calculateCompressionSizeLimit(fileContext, uncompressedBlockSize); | ||
| return uncompressedBlockSize >= adjustedBlockSize; | ||
| } | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
Contributor
There was a problem hiding this comment.
I'll also move this to Predicator and generalize this method
Suggested change
| publicbooleanshouldFinishBlock() throwsIOException { | |
| // int uncompressedBlockSize = blockSizeWritten(); | |
| intuncompressedBlockSize = baosInMemory.size(); | |
| if (uncompressedBlockSize >= fileContext.getBlocksize()) { | |
| if (uncompressedBlockSize < maxSizeUnCompressed) { | |
| intadjustedBlockSize = compressedSizePredicator. | |
| calculateCompressionSizeLimit(fileContext, uncompressedBlockSize); | |
| returnuncompressedBlockSize >= adjustedBlockSize; | |
| } | |
| returntrue; | |
| } | |
| returnfalse; | |
| } | |
| publicbooleancheckBoundariesWithPredicate() throwsIOException { | |
| if(predicator==null){ | |
| thrownewIllegalArgumentException("Expected at least the default BoundariesCheckPredicate"); | |
| } | |
| returnpredicator. | |
| shouldFinishBlock(fileContext, uncompressedBlockSize); | |
| } | |
| */ | ||
| @Override | ||
| public void updateLatestBlockSizes(int uncompressed, int compressed) { | ||
| compressionRatio = uncompressed/compressed; |
Contributor
There was a problem hiding this comment.
nit:
Suggested change
| compressionRatio = uncompressed/compressed; | |
| compressionFactor = uncompressed/compressed; |
Comment on lines
+1266
to
+1267
| System.out.println(">>>> " + block.getUncompressedSizeWithoutHeader()); | ||
| System.out.println(">>>> " + blockCount); |
Apache-HBase
commented
Aug 15, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 15, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 15, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 15, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 15, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 15, 2022
🎊 +1 overall
This message was automatically generated. |
wchevreuil added a commit
that referenced
this pull request
Aug 15, 2022
…locks during hfile writes (#4675) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Signed-off-by: Ankit Singhal <ankit@apache.org>
vinayakphegde pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…locks during hfile writes (apache#4675) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Signed-off-by: Ankit Singhal <ankit@apache.org> Change-Id: I0d969c83f9bb6a2a8d24e60e6b76b412b659e2b0
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
6 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.
Here we propose two additional properties,"hbase.block.size.limit.compressed" and "hbase.block.size.max.compressed" that would allow for consider the compressed size (if compression is in use) for delimiting blocks during hfile writing. When compression is enabled, certain datasets can have very high compression efficiency, so that the default 64KB block size and 10GB max file size can lead to hfiles with very large number of blocks.
In this proposal, "hbase.block.size.limit.compressed" is a boolean flag that switches to compressed size for delimiting blocks, and "hbase.block.size.max.compressed" is an int with the limit, in bytes for the compressed block size, in order to avoid very large uncompressed blocks (defaulting to 320KB).