feat(config): add `rocksdb.compression_start_level` to allow configure levels without compression #2605

paragor · 2024-10-17T08:21:19Z

references: #2602

add rocksdb.compression_start_level to allow configure levels without compression

kvrocks.conf

PragmaTwice · 2024-10-17T15:03:00Z

From the CI log, we need to add vector.reserve() before the loop containing vector.push_back().

paragor · 2024-10-17T15:16:04Z

From the CI log, we need to add vector.reserve() before the loop containing vector.push_back().

thank you, didnt see it.
fixed

PragmaTwice · 2024-10-17T16:27:42Z

src/config/config.cc

+    for (int i = 0; i < config->rocks_db.nocompression_for_first_n_levels; i++) {
+      compression_levels_builder.emplace_back("kNoCompression");
+    }
+    for (size_t i = config->rocks_db.nocompression_for_first_n_levels;


We need to check at the beginning of the callback that nocompression_for_first_n_levels <= compression_per_level.size().

Also you can make a variable for compression_per_level.size().

i was thinking that boundary check is unnecessary, because kvrocks has hardcoded 7 lvles of compactions. but it is ok, i add it

PragmaTwice · 2024-10-17T16:28:45Z

src/config/config.cc

@@ -201,6 +201,8 @@ Config::Config() {
       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)},
+      {"rocksdb.nocompression_for_first_n_levels", false,
+       new IntField(&rocks_db.nocompression_for_first_n_levels, 2, 0, 7)},


I'm wondering why it's 7 here? maybe we can add some comments.

kvrocks support only 7 levels of leveled compaction, ok i stay a comment

I think rather than a comment, we can use a constexpr valuable like KVROCKS_MAX_LSM_LEVEL = 7, and use it in config and here

where should i declare const? in config.h or storage.h?

oh i see config.h best place for it

Ok to be in config.h

Beihao-Zhou · 2024-10-17T23:13:46Z

src/config/config.cc

-      if (option.name == v) {
+      if (option.type == srv->GetConfig()->rocks_db.compression) {


Just wondering if this change here is just to make the comparison condition more concrete, or it has to be changed to make nocompression_for_first_levels work? Thank you!

previously it compare value that come to callback. callback was used once when "compression" option set. Now callback call twice - for set "compression" and for set "nocompression_for_first_n_levels", so value may be name of compression or levels count that no need compression.

if i make two separate callback, it will have dublicate code, because this two options closely related.

i dont know how make this code better, if you have solution, plz share with me :)
i never write in c++ before

This is a bit weird to reuse the same function...@git-hulk do you have any better idea here? There're 2 configs about compression and would set the system once...

I have no better solution for now because both two configurations will affect the same rocksdb option. It's weird while compared to other configurations, but it looks reasonable.

Should we separate it to:

set_compression_method_callback set_no_compression_level_callback

?

@mapleFU Do you mean this?

rocksdb::Status set_rocksdb_compression(CompressionType compression, int nocompression_for_first_n_levels) { ... } rocksdb::Status set_compression_method_callback() { ... set_rocksdb_compression(compression, nocompression_for_first_n_levels) } rocksdb::Status set_no_compression_level_callback() { ... set_rocksdb_compression(compression, nocompression_for_first_n_levels) }

If yes, I'm good with it.

i'am do it in separate commit, but i dont see any benefits. validation of fields is done already in IntField and EnumField<rocksdb::CompressionType>, so no need validate options again.
set_db_option_cb used more then 1 times for example

paragor · 2024-10-18T06:14:08Z

@PragmaTwice what do you think about rename nocompression_for_first_n_levels -> start_compression_level?

mapleFU

So AFAIK, just like #2607 . You'd like to enable compression on L1 ( or even L0 ) for write intensive case?

src/config/config.cc

src/storage/storage.cc

mapleFU · 2024-10-18T06:46:06Z

Perhaps, if you're a heavy write system, maybe you can enable different compression in different level, like lz4 in low level and zstd in high level...

wanghenshui · 2024-10-18T06:55:52Z

maybe we could specific compression setting by level? user could mark l0/l1 as snappy or lz4 compression style

l0/l1 nocompression is common type of usage, use or not,

rocksdb.nocompression_for_first_n_levels indicate user could make l2/l3... no compression, user may not know about rocksdb stuff, make them confused about what senerio to use it

instead, let user specifc what compression style level used, maybe more clear.

paragor · 2024-10-18T07:40:47Z

So AFAIK, just like #2607 . You'd like to enable compression on L1 ( or even L0 ) for write intensive case?

yes, write 128MB compressed data on disk is better, than write uncompressed 1 GB data on disk. Now cpu usage is only 1/8 cores. so i need able to load cpu and decrease disk usage.

it is not general case, but in my case it will help me to use disks with fewer throughput and as a consequence of ec2 limitations - cheaper ec2 instance

paragor · 2024-10-18T07:50:41Z

maybe we could specific compression setting by level? user could mark l0/l1 as snappy or lz4 compression style

I think this is a good point
Something similar to the more advanced settings in kvrocks. By default, the "compression" settings are used, but if the new setting "compression_by_levels" is specified, it takes precedence, and the "compression" setting is ignored.

mapleFU · 2024-10-18T07:54:07Z

Maybe we should first check this in, would you mind fix the comments firstly?

…onfigure levels without compression

mapleFU · 2024-10-18T10:31:00Z

kvrocks.conf

+# By default disabled for the first two levels, because it may contain the
+# frequently accessed data, so it'd be better to use uncompressed data to
+# save the CPU.
+# Value: [0, 7] (upper boundary is rocksdb levels count)


Suggested change

# Value: [0, 7] (upper boundary is rocksdb levels count)

# Value: [0, 7] (upper boundary is kvrocks maximum levels count)

Would above being better?

mapleFU · 2024-10-18T10:31:46Z

src/config/config.cc

@@ -201,6 +201,8 @@ Config::Config() {
       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)},
+      {"rocksdb.nocompression_for_first_n_levels", false,
+       new IntField(&rocks_db.nocompression_for_first_n_levels, 2, 0, 7)},


I think rather than a comment, we can use a constexpr valuable like KVROCKS_MAX_LSM_LEVEL = 7, and use it in config and here

mapleFU · 2024-10-18T10:37:49Z

src/config/config.cc

-      if (option.name == v) {
+      if (option.type == srv->GetConfig()->rocks_db.compression) {


This is a bit weird to reuse the same function...@git-hulk do you have any better idea here? There're 2 configs about compression and would set the system once...

PragmaTwice · 2024-10-18T11:35:13Z

@PragmaTwice what do you think about rename nocompression_for_first_n_levels -> start_compression_level?

Nice. But seems compression_start_level or compression_begin_level is better to me.

…dd const KVROCKS_MAX_LSM_LEVEL

paragor · 2024-10-21T08:14:47Z

@PragmaTwice what do you think about rename nocompression_for_first_n_levels -> start_compression_level?

Nice. But seems compression_start_level or compression_begin_level is better to me.

renamed

sonarqubecloud · 2024-10-21T11:40:19Z

Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
51.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mapleFU

LGTM, thanks!

src/config/config.cc

mapleFU · 2024-10-21T16:27:00Z

src/config/config.cc

+  if (compression_start_level >= KVROCKS_MAX_LSM_LEVEL) {
+    return {Status::NotOK, "compression_start_level must <= rocksdb levels count"};


Oh I catch a problem here, should we weeken to !(compression_start_level > KVROCKS_MAX_LSM_LEVEL)?

Oh here we should just change content to return {Status::NotOK, "compression_start_level must < rocksdb levels count"};

aleksraiden requested review from torwig, git-hulk, PragmaTwice and Beihao-Zhou October 17, 2024 08:23

PragmaTwice reviewed Oct 17, 2024

View reviewed changes

kvrocks.conf Outdated Show resolved Hide resolved

paragor force-pushed the add_config_nocompression_for_first_levels branch from 7001a61 to 198699e Compare October 17, 2024 10:04

paragor requested a review from PragmaTwice October 17, 2024 11:21

paragor force-pushed the add_config_nocompression_for_first_levels branch from 198699e to 2f03090 Compare October 17, 2024 12:28

paragor force-pushed the add_config_nocompression_for_first_levels branch from 2f03090 to e49ff75 Compare October 17, 2024 15:15

PragmaTwice reviewed Oct 17, 2024

View reviewed changes

paragor force-pushed the add_config_nocompression_for_first_levels branch 2 times, most recently from 8f7f87d to 0268b9c Compare October 17, 2024 17:32

paragor requested a review from PragmaTwice October 17, 2024 17:32

paragor force-pushed the add_config_nocompression_for_first_levels branch from 0268b9c to 5fb1224 Compare October 17, 2024 17:49

Beihao-Zhou reviewed Oct 17, 2024

View reviewed changes

paragor force-pushed the add_config_nocompression_for_first_levels branch from 5fb1224 to 590becd Compare October 18, 2024 05:52

PragmaTwice changed the title ~~feat(config): add rocksdb.nocompression_for_first_levels to allow configure levels without compression~~ feat(config): add rocksdb.nocompression_for_first_n_levels to allow configure levels without compression Oct 18, 2024

PragmaTwice requested a review from mapleFU October 18, 2024 06:11

mapleFU reviewed Oct 18, 2024

View reviewed changes

src/config/config.cc Outdated Show resolved Hide resolved

src/storage/storage.cc Outdated Show resolved Hide resolved

feat(config): add rocksdb.nocompression_for_first_levels to allow c…

9cef6d6

…onfigure levels without compression

paragor force-pushed the add_config_nocompression_for_first_levels branch from 590becd to 9cef6d6 Compare October 18, 2024 09:07

paragor requested a review from mapleFU October 18, 2024 09:08

mapleFU reviewed Oct 18, 2024

View reviewed changes

paragor added 2 commits October 21, 2024 14:16

rename nocompression_for_first_n_levels -> compression_start_level, a…

4ba502c

…dd const KVROCKS_MAX_LSM_LEVEL

split set compression callback into two separate one

c804f02

paragor changed the title ~~feat(config): add rocksdb.nocompression_for_first_n_levels to allow configure levels without compression~~ feat(config): add rocksdb.compression_start_level to allow configure levels without compression Oct 21, 2024

paragor requested a review from mapleFU October 21, 2024 08:15

Merge branch 'unstable' into add_config_nocompression_for_first_levels

ea00eda

git-hulk approved these changes Oct 21, 2024

View reviewed changes

git-hulk requested a review from Beihao-Zhou October 21, 2024 11:42

mapleFU approved these changes Oct 21, 2024

View reviewed changes

PragmaTwice reviewed Oct 21, 2024

View reviewed changes

src/config/config.cc Show resolved Hide resolved

PragmaTwice approved these changes Oct 21, 2024

View reviewed changes

PragmaTwice merged commit e0de934 into apache:unstable Oct 21, 2024
32 checks passed

mapleFU reviewed Oct 21, 2024

View reviewed changes

		if (option.name == v) {
		if (option.type == srv->GetConfig()->rocks_db.compression) {

	# Value: [0, 7] (upper boundary is rocksdb levels count)
	# Value: [0, 7] (upper boundary is kvrocks maximum levels count)

		if (compression_start_level >= KVROCKS_MAX_LSM_LEVEL) {
		return {Status::NotOK, "compression_start_level must <= rocksdb levels count"};

feat(config): add rocksdb.compression_start_level to allow configure levels without compression #2605

feat(config): add rocksdb.compression_start_level to allow configure levels without compression #2605

Conversation

paragor commented Oct 17, 2024 • edited Loading

PragmaTwice commented Oct 17, 2024

paragor commented Oct 17, 2024

Choose a reason for hiding this comment

paragor Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paragor Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

paragor commented Oct 18, 2024

mapleFU left a comment

Choose a reason for hiding this comment

mapleFU commented Oct 18, 2024

wanghenshui commented Oct 18, 2024

paragor commented Oct 18, 2024 • edited Loading

paragor commented Oct 18, 2024

mapleFU commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice commented Oct 18, 2024 • edited Loading

paragor commented Oct 21, 2024

sonarqubecloud bot commented Oct 21, 2024

Quality Gate passed

mapleFU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(config): add `rocksdb.compression_start_level` to allow configure levels without compression #2605

feat(config): add `rocksdb.compression_start_level` to allow configure levels without compression #2605

paragor commented Oct 17, 2024 •

edited

Loading

paragor Oct 17, 2024 •

edited

Loading

paragor Oct 21, 2024 •

edited

Loading

paragor commented Oct 18, 2024 •

edited

Loading

PragmaTwice commented Oct 18, 2024 •

edited

Loading