HBASE-26316 Per-table or per-CF compression codec setting overrides #3730
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. |
|
Confirmed functionality with a single host cluster: Loaded one WARC from common crawl. Major compaction takes 11 seconds. Major compaction now takes 42 seconds. Total size on disk reduced by 11.6% vs level 1. Major compaction now takes 17 minutes 15 seconds. Total size on disk reduced by 13.3% vs level 1. (Sure, this level is crazy in practice.) |
|
🎊 +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. |
apurtell
force-pushed
the
HBASE-26316
branch
2 times, most recently
from
3184470
to
c7e6996
Compare
|
Rebase and update commit message. |
|
🎊 +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. |
|
The test failures are not related and appear environmental in nature. I ran the tests locally with this change applied and they passed. TestQuotaAdmin seems like a resource intensive unit (has a running time of 86 seconds on my macbook) and maybe we should break it up. |
|
Rebase. Fix checkstyle warnings. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
More unstable tests, but they do not seem related to this change: One error Three flakes: |
|
🎊 +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. |
|
Test failures are not related |
|
@virajjasani @Apache9 This PR is blocking #3748, which has been approved for merge. I refactored the changes of #3748 and this PR to make #3748 smaller and collect all of the the changes for plumbing There might be another round of checkstyle fixes needed. Will apply them as soon as the precommit report is available. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
apurtell
added a commit
to apurtell/hbase
that referenced
this pull request
Oct 18, 2021
This will cause a small merge conflict between apache#3730 and apache#3748 because we need CanReinit here too.
We get and retain Compressor instances in HFileBlockDefaultEncodingContext, and could in theory call Compressor#reinit when setting up the context, to update compression parameters like level and buffer size, but we do not plumb through the CompoundConfiguration from the Store into the encoding context. As a consequence we can only update codec parameters globally in system site conf files. Fine grained configurability is important for algorithms like ZStandard (ZSTD), which offers more than 20 compression levels, where at level 1 it is almost as fast as LZ4, and where at higher levels it utilizes computationally expensive techniques to rival LZMA at compression ratio but trades off significantly for reduced compresson throughput. The ZSTD level that should be set for a given column family or table will vary by use case.
apurtell
added a commit
to apurtell/hbase
that referenced
this pull request
Oct 19, 2021
This will cause a small merge conflict between apache#3730 and apache#3748 because we need CanReinit here too.
|
Rebase |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
approved these changes
Oct 19, 2021
| } | ||
|
|
||
| @Override | ||
| public HFileBlockDecodingContext newDataBlockDecodingContext(HFileContext meta) { | ||
| return new HFileBlockDefaultDecodingContext(meta); | ||
| public HFileBlockDecodingContext newDataBlockDecodingContext(Configuration conf, HFileContext meta) { |
There was a problem hiding this comment.
nit: checkstyle line-length might complain
| * @param meta | ||
| * @param rawKVs raw KVs | ||
| * @param meta hfile context | ||
| * @param conf store configuration |
There was a problem hiding this comment.
nit: looks like duplicate @param, it's already covered above
| decompressor = compression.getDecompressor(); | ||
| // Some algorithms don't return decompressors and accept null as a valid parameter for | ||
| // same when creating decompression streams. We can ignore these cases wrt reinit. | ||
| if (decompressor != null && decompressor instanceof CanReinit) { |
There was a problem hiding this comment.
Simplify to decompressor instanceof CanReinit as it covers null check also?
There was a problem hiding this comment.
I didn't know that, cool.
Comment on lines
+103
to
+112
| try (InputStream is = | ||
| compression.createDecompressionStream(dataInputStream, decompressor, 0)) { | ||
| BlockIOUtils.readFullyWithHeapBuffer(is, blockBufferWithoutHeader, | ||
| uncompressedSizeWithoutHeader); | ||
| } | ||
| } finally { | ||
| if (decompressor != null) { | ||
| compression.returnDecompressor(decompressor); | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: shall we remove Compression#decompress method? After this change, perhaps it is not being used anywhere else, nothing urgent, can be follow up too if it creates complexities.
There was a problem hiding this comment.
Sure, it is not being used, I think removal is a good idea
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
asfgit
pushed a commit
that referenced
this pull request
Oct 19, 2021
…3730) We get and retain Compressor instances in HFileBlockDefaultEncodingContext, and could in theory call Compressor#reinit when setting up the context, to update compression parameters like level and buffer size, but we do not plumb through the CompoundConfiguration from the Store into the encoding context. As a consequence we can only update codec parameters globally in system site conf files. Fine grained configurability is important for algorithms like ZStandard (ZSTD), which offers more than 20 compression levels, where at level 1 it is almost as fast as LZ4, and where at higher levels it utilizes computationally expensive techniques to rival LZMA at compression ratio but trades off significantly for reduced compresson throughput. The ZSTD level that should be set for a given column family or table will vary by use case. Signed-off-by: Viraj Jasani <vjasani@apache.org> Conflicts: hbase-compression/hbase-compression-zstd/src/main/java/org/apache/hadoop/hbase/io/compress/zstd/ZstdDecompressor.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/compress/HFileTestBase.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/encoding/TestSeekToBlockWithEncoders.java
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.
We get and retain Compressor instances in HFileBlockDefaultEncodingContext, and could in theory call Compressor#reinit when setting up the context, to update compression parameters like level and buffer size, but we do not plumb through the CompoundConfiguration from the Store into the encoding context. As a consequence we can only update codec parameters globally in system site conf files.
Fine grained configurability is important for algorithms like ZStandard (ZSTD), which offers more than 20 compression levels, where at level 1 it is almost as fast as LZ4, and where at higher levels it utilizes computationally expensive techniques to rival LZMA at compression ratio but trades off significantly for reduced compresson throughput. The ZSTD level that should be set for a given column family or table will vary by use case.