You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "Beihao-Zhou (via GitHub)" <gi...@apache.org> on 2024/03/14 21:28:47 UTC
[PR] Add compression level option [kvrocks]
Beihao-Zhou opened a new pull request, #2171:
URL: https://github.com/apache/kvrocks/pull/2171
close: #2143
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1526571483
##########
src/config/config.cc:
##########
@@ -190,6 +190,7 @@ Config::Config() {
{"rocksdb.compression", false,
new EnumField<rocksdb::CompressionType>(&rocks_db.compression, compression_types,
rocksdb::CompressionType::kNoCompression)},
+ {"rocksdb.compression_level", true, new IntField(&rocks_db.compression_level, 32767, INT_MIN, INT_MAX)},
Review Comment:
aha, my bad. I mean why it's different from compression, which could be changed dynamically.
This looks ok to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1526400770
##########
kvrocks.conf:
##########
@@ -727,6 +727,27 @@ rocksdb.cache_index_and_filter_blocks yes
# default snappy
rocksdb.compression snappy
+# Specify the compression level to use. It trades compression speed
+# and ratio, might be useful when tuning for disk space.
+# See details: https://github.com/facebook/rocksdb/wiki/Space-Tuning
+# For zstd: valid range is from 1 to 19,
+# where 1 is fastest compression and 19 is best compression.
+# For zlib: valid range is from 1 to 9,
+# where 1 is fastest compression and 9 is best compression.
+# For lz4: adjusting the level influences the 'acceleration'.
+# RocksDB sets a negative level to indicate acceleration directly,
+# with more negative values indicating higher speed and less compression.
+# Note: This setting is ignored for compression algorithms like Snappy that
+# do not support variable compression levels.
+# RocksDB Default:
+# - zstd: 3
+# - zlib: Z_DEFAULT_COMPRESSION (currently -1)
+# - kLZ4: -1 (i.e., `acceleration=1`; see `CompressionOptions::level` doc)
+# For all others, RocksDB does not specify a compression level.
+# If the compression type doesn't support the setting, it will be a no-op.
+# Default: 6 (DB space tuning)
+rocksdb.compression_level 6
Review Comment:
Sure thing! Set `kDefaultCompressionLevel = 32767` as default and add comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1526495152
##########
src/config/config.cc:
##########
@@ -190,6 +190,7 @@ Config::Config() {
{"rocksdb.compression", false,
new EnumField<rocksdb::CompressionType>(&rocks_db.compression, compression_types,
rocksdb::CompressionType::kNoCompression)},
+ {"rocksdb.compression_level", true, new IntField(&rocks_db.compression_level, 32767, INT_MIN, INT_MAX)},
Review Comment:
Seems like RocksDB just hardcode this value 32767 into their compression level logic.
Zlib: https://github.com/facebook/rocksdb/blob/096fb9b67d19a9a180e7c906b4a0cdb2b2d0c1f6/util/compression.h#L805
Zstd: https://github.com/facebook/rocksdb/blob/096fb9b67d19a9a180e7c906b4a0cdb2b2d0c1f6/util/compression.h#L183
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1525635390
##########
kvrocks.conf:
##########
@@ -911,3 +930,5 @@ rocksdb.rate_limiter_auto_tuned yes
################################ NAMESPACE #####################################
# namespace.test change.me
+backup-dir /tmp/kvrocks/backup
+pidfile /tmp/kvrocks/kvrocks.pid
Review Comment:
Yes, I will take a look at why those two lines are generated after running.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1526442900
##########
src/config/config.cc:
##########
@@ -190,6 +190,7 @@ Config::Config() {
{"rocksdb.compression", false,
new EnumField<rocksdb::CompressionType>(&rocks_db.compression, compression_types,
rocksdb::CompressionType::kNoCompression)},
+ {"rocksdb.compression_level", true, new IntField(&rocks_db.compression_level, 32767, INT_MIN, INT_MAX)},
Review Comment:
🤔why compression_level can be dynamically changed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1526251122
##########
kvrocks.conf:
##########
@@ -727,6 +727,25 @@ rocksdb.cache_index_and_filter_blocks yes
# default snappy
rocksdb.compression snappy
+# Specify the compression level to use.
Review Comment:
Sure, add both comment and link in the updated PR :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1525703123
##########
kvrocks.conf:
##########
@@ -727,6 +727,25 @@ rocksdb.cache_index_and_filter_blocks yes
# default snappy
rocksdb.compression snappy
+# Specify the compression level to use.
+# For zstd: valid range is from 1 to 19,
+# where 1 is fastest compression and 19 is best compression.
+# For zlib: valid range is from 1 to 9,
+# where 1 is fastest compression and 9 is best compression.
+# For lz4: adjusting the level influences the 'acceleration'.
+# RocksDB sets a negative level to indicate acceleration directly,
+# with more negative values indicating higher speed and less compression.
+# Note: This setting is ignored for compression algorithms like Snappy that
+# do not support variable compression levels.
+# RocksDB Default:
+# - zstd: 3
+# - zlib: Z_DEFAULT_COMPRESSION (currently -1)
+# - kLZ4: -1 (i.e., `acceleration=1`; see `CompressionOptions::level` doc)
+# For all others, RocksDB does not specify a compression level.
+# If the compression type doesn't support the setting, it will be a no-op.
+# Default: 6 DB space tuning
Review Comment:
`DB space tuning` What does that mean?
##########
kvrocks.conf:
##########
@@ -727,6 +727,25 @@ rocksdb.cache_index_and_filter_blocks yes
# default snappy
rocksdb.compression snappy
+# Specify the compression level to use.
Review Comment:
I thought it would be better to add: `It trades compression speed and ratio, might be useful when tuning for disk space`.
Or we attach https://github.com/facebook/rocksdb/wiki/Space-Tuning link to guide users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1526258669
##########
kvrocks.conf:
##########
@@ -727,6 +727,27 @@ rocksdb.cache_index_and_filter_blocks yes
# default snappy
rocksdb.compression snappy
+# Specify the compression level to use. It trades compression speed
+# and ratio, might be useful when tuning for disk space.
+# See details: https://github.com/facebook/rocksdb/wiki/Space-Tuning
+# For zstd: valid range is from 1 to 19,
+# where 1 is fastest compression and 19 is best compression.
+# For zlib: valid range is from 1 to 9,
+# where 1 is fastest compression and 9 is best compression.
+# For lz4: adjusting the level influences the 'acceleration'.
+# RocksDB sets a negative level to indicate acceleration directly,
+# with more negative values indicating higher speed and less compression.
+# Note: This setting is ignored for compression algorithms like Snappy that
+# do not support variable compression levels.
+# RocksDB Default:
+# - zstd: 3
+# - zlib: Z_DEFAULT_COMPRESSION (currently -1)
+# - kLZ4: -1 (i.e., `acceleration=1`; see `CompressionOptions::level` doc)
+# For all others, RocksDB does not specify a compression level.
+# If the compression type doesn't support the setting, it will be a no-op.
+# Default: 6 (DB space tuning)
+rocksdb.compression_level 6
Review Comment:
https://github.com/facebook/rocksdb/blob/096fb9b67d19a9a180e7c906b4a0cdb2b2d0c1f6/include/rocksdb/compression_type.h#L43-L68
Why setting it to 6 by default？Though the doc says 6 is space tuning ( https://github.com/facebook/rocksdb/wiki/Space-Tuning#compression-level ), I think we should keep it "default in rocksdb" by default
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1526250463
##########
kvrocks.conf:
##########
@@ -911,3 +930,5 @@ rocksdb.rate_limiter_auto_tuned yes
################################ NAMESPACE #####################################
# namespace.test change.me
+backup-dir /tmp/kvrocks/backup
+pidfile /tmp/kvrocks/kvrocks.pid
Review Comment:
Removed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1527030452
##########
src/config/config.cc:
##########
@@ -190,6 +190,7 @@ Config::Config() {
{"rocksdb.compression", false,
new EnumField<rocksdb::CompressionType>(&rocks_db.compression, compression_types,
rocksdb::CompressionType::kNoCompression)},
+ {"rocksdb.compression_level", true, new IntField(&rocks_db.compression_level, 32767, INT_MIN, INT_MAX)},
Review Comment:
Rocksdb don't have this interface is fair enough, however, I don't think changing the level need re-encoding. And decompress block also doesn't need the level
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#issuecomment-1998793881
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2171) **Quality Gate failed**
Failed conditions
 [8.6% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2171&metric=new_duplicated_lines_density&view=list) (required ≤ 5%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2171)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1527166817
##########
src/config/config.cc:
##########
@@ -190,6 +190,7 @@ Config::Config() {
{"rocksdb.compression", false,
new EnumField<rocksdb::CompressionType>(&rocks_db.compression, compression_types,
rocksdb::CompressionType::kNoCompression)},
+ {"rocksdb.compression_level", true, new IntField(&rocks_db.compression_level, 32767, INT_MIN, INT_MAX)},
Review Comment:
I see I see, thank you very much for correcting me and finding the reference!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1526495152
##########
src/config/config.cc:
##########
@@ -190,6 +190,7 @@ Config::Config() {
{"rocksdb.compression", false,
new EnumField<rocksdb::CompressionType>(&rocks_db.compression, compression_types,
rocksdb::CompressionType::kNoCompression)},
+ {"rocksdb.compression_level", true, new IntField(&rocks_db.compression_level, 32767, INT_MIN, INT_MAX)},
Review Comment:
Seems like RocksDB just hardcode this value 32767 into their compression level logic.
exposed level option const: https://github.com/facebook/rocksdb/blob/096fb9b67d19a9a180e7c906b4a0cdb2b2d0c1f6/include/rocksdb/compression_type.h#L53
Zlib: https://github.com/facebook/rocksdb/blob/096fb9b67d19a9a180e7c906b4a0cdb2b2d0c1f6/util/compression.h#L805
Zstd: https://github.com/facebook/rocksdb/blob/096fb9b67d19a9a180e7c906b4a0cdb2b2d0c1f6/util/compression.h#L183
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1527031977
##########
src/config/config.cc:
##########
@@ -190,6 +190,7 @@ Config::Config() {
{"rocksdb.compression", false,
new EnumField<rocksdb::CompressionType>(&rocks_db.compression, compression_types,
rocksdb::CompressionType::kNoCompression)},
+ {"rocksdb.compression_level", true, new IntField(&rocks_db.compression_level, 32767, INT_MIN, INT_MAX)},
Review Comment:
Besides, maybe refer to https://github.com/facebook/rocksdb/pull/6615 helps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1525630414
##########
kvrocks.conf:
##########
@@ -911,3 +930,5 @@ rocksdb.rate_limiter_auto_tuned yes
################################ NAMESPACE #####################################
# namespace.test change.me
+backup-dir /tmp/kvrocks/backup
+pidfile /tmp/kvrocks/kvrocks.pid
Review Comment:
These two lines should not be committed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#issuecomment-1999970993
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2171) **Quality Gate failed**
Failed conditions
 [8.6% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2171&metric=new_duplicated_lines_density&view=list) (required ≤ 5%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2171)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "Beihao-Zhou (via GitHub)" <gi...@apache.org>.
Beihao-Zhou commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1526711453
##########
src/config/config.cc:
##########
@@ -190,6 +190,7 @@ Config::Config() {
{"rocksdb.compression", false,
new EnumField<rocksdb::CompressionType>(&rocks_db.compression, compression_types,
rocksdb::CompressionType::kNoCompression)},
+ {"rocksdb.compression_level", true, new IntField(&rocks_db.compression_level, 32767, INT_MIN, INT_MAX)},
Review Comment:
Oh gotcha, nwnw, I did have the same question while I was working on it.
So when we change the compression type, it doesn't require re-encoding of the existing data because each block or SST file includes metadata specifying the compression algorithm used for that particular block or file. This allows RocksDB to understand how to decompress the data regardless of the global compression settings at any given time.
However, compression level, unlike compression types, which are recorded and recognized on a per-block or per-file basis, the specific level detail isn't stored with each block or file. Therefore, if we want to change it, it will require re-encoding everything, which is a huge overhead. I think that's the reason why RocksDB doesn't allow us to dynamically SetOption() for it.
Ref: https://github.com/facebook/rocksdb/wiki/Compression (The first line explains a bit about the granularity of encoding block)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add compression level option [kvrocks]
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #2171:
URL: https://github.com/apache/kvrocks/pull/2171#discussion_r1526304155
##########
kvrocks.conf:
##########
@@ -727,6 +727,27 @@ rocksdb.cache_index_and_filter_blocks yes
# default snappy
rocksdb.compression snappy
+# Specify the compression level to use. It trades compression speed
+# and ratio, might be useful when tuning for disk space.
+# See details: https://github.com/facebook/rocksdb/wiki/Space-Tuning
+# For zstd: valid range is from 1 to 19,
+# where 1 is fastest compression and 19 is best compression.
+# For zlib: valid range is from 1 to 9,
+# where 1 is fastest compression and 9 is best compression.
+# For lz4: adjusting the level influences the 'acceleration'.
+# RocksDB sets a negative level to indicate acceleration directly,
+# with more negative values indicating higher speed and less compression.
+# Note: This setting is ignored for compression algorithms like Snappy that
+# do not support variable compression levels.
+# RocksDB Default:
+# - zstd: 3
+# - zlib: Z_DEFAULT_COMPRESSION (currently -1)
+# - kLZ4: -1 (i.e., `acceleration=1`; see `CompressionOptions::level` doc)
+# For all others, RocksDB does not specify a compression level.
+# If the compression type doesn't support the setting, it will be a no-op.
+# Default: 6 (DB space tuning)
+rocksdb.compression_level 6
Review Comment:
I agree. I also think we should use the rocksdb default value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org