Add a option to specify write_buffer_size for minor column families #2258
Conversation
git-hulk
requested review from
git-hulk,
PragmaTwice and
torwig
and removed request for
git-hulk
|
@jjz921024 Thanks for your contributions. |
|
I believe "lite mode" is not a suitable name as it does not clearly convey its purpose. We should find a more specific name instead of labeling any random behavior as "lite mode". |
|
Maybe we can rename this option to |
src/storage/storage.cc
Outdated
| lite_opts.write_buffer_size = 16 * KiB; | ||
| pubsub_opts.write_buffer_size = 16 * KiB; | ||
| propagate_opts.write_buffer_size = 16 * KiB; | ||
| } |
It can be |
jjz921024
changed the title
jjz921024
changed the title
|
@jjz921024 I'm good with this implementation. |
| @@ -119,6 +119,7 @@ struct Config { | |||
| bool auto_resize_block_and_sst = true; | |||
| int fullsync_recv_file_delay = 0; | |||
| bool use_rsid_psync = false; | |||
| int minor_columns_write_buffer_size; | |||
There was a problem hiding this comment.
I'm so sorry that I was so busy these days. I've checked this patch. Should we add a tag for kvrocks owned Column Familys, and mark some CF as "minor"?
This patch looks ok, however, when adjust the rocksdb.write_buffer_size, the buffer size for minor would be overwrite. Besides, our code enforce "write_buffer_size" in some scenerio
I'm also OK with this as a first patch here.
src/config/config.cc
Outdated
| @@ -189,6 +190,7 @@ Config::Config() { | |||
| {"json-max-nesting-depth", false, new IntField(&json_max_nesting_depth, 1024, 0, INT_MAX)}, | |||
| {"json-storage-format", false, | |||
| new EnumField<JsonStorageFormat>(&json_storage_format, json_storage_formats, JsonStorageFormat::JSON)}, | |||
| {"minor-columns-write-buffer-size", false, new IntField(&minor_columns_write_buffer_size, 65536, 16, 4194304)}, | |||
There was a problem hiding this comment.
Besides, would this conflict with rocksdb.write_buffer_size? Should we denote that?
There was a problem hiding this comment.
Yes, If the user just want to adjust write_buffer_size for subkey and keep other minor cf. The user had to execute 2 config command.
- first exec
config set rocksdb.write_buffer_size numto adjust all cf - and exec
config set minor-columns-write-buffer-size numfor recover minor cf
we should denote it in comment
@git-hulk @PragmaTwice what do your think?
| {"minor-columns-write-buffer-size", | ||
| [this](Server *srv, const std::string &k, const std::string &v) -> Status { | ||
| if (!srv) return Status::OK(); | ||
| const std::vector<ColumnFamilyID> column_families = {kColumnFamilyIDZSetScore, kColumnFamilyIDPubSub, |
There was a problem hiding this comment.
Should we limit the CF set by "rocksdb.write_buffer_size"?
src/config/config.cc
Outdated
| kColumnFamilyIDSearch}; | ||
| for (const auto &cf : column_families) { | ||
| auto s = srv->storage->SetOptionForColumnFamily(cf, "write_buffer_size", | ||
| std::to_string(minor_columns_write_buffer_size * KiB)); |
There was a problem hiding this comment.
Should this be an argument of rocks_db?
There was a problem hiding this comment.
Do you mean prefix this option with rocksdb，like rocksdb.minor-columns-write-buffer-size ?
I'm struggling with this. Because all options prefix with rocksdb correspond to the options of rocksdb, but this is a behavior of kvrocks.
There was a problem hiding this comment.
Because all options prefix with rocksdb correspond to the options of rocksdb, but this is a behavior of kvrocks.
rocksdb.write_buffer_size previously is also a part of rocksdb and written inside config.rocks_db, maybe we should regard them as same thing?
src/storage/storage.cc
Outdated
| pubsub_opts.write_buffer_size = config_->minor_columns_write_buffer_size * KiB; | ||
| propagate_opts.write_buffer_size = config_->minor_columns_write_buffer_size * KiB; |
There was a problem hiding this comment.
Could you explain for these two lines of code?
There was a problem hiding this comment.
Don't we want to specify write_buffer_size of all minor column families except for metadata and subkey?
There was a problem hiding this comment.
What will happen if minor_columns_write_buffer_size is disabled?
| # It will reduce the memory usage in some scenarios. | ||
| # | ||
| # Default: 64 MB (same with rocksdb.write_buffer_size) | ||
| minor-columns-write-buffer-size 64 |
There was a problem hiding this comment.
I think by default it can be 0, which means its value as same as rocksdb.write_buffer_size.
|
@jjz921024 Maybe a simpler way is:
Personally (3) is a bit hard but I think these CF should be tuned themselves in the future, maintaining two configs both for "minor"( like if "rocksdb.write_buffer_size" is set and "rocksdb.minor_write_buffer_size" doesn't set, using "rocksdb.write_buffer_size", and otherwise separate them) is better for users, but might be a bit tricky. Maybe we can separate it into other patch? Thanks for the efforts! |
For #2193
Add a
lite_modeoption to special kvrock running at lite_mode,At this mode, the
lite_optswill replace forsubkey_optson some infrequently used column familiesfor reduce memory usage in some scenarios.