Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a option to specify write_buffer_size for minor column families #2258

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from

Conversation

jjz921024
Copy link
Contributor

For #2193

Add a lite_mode option to special kvrock running at lite_mode,
At this mode, the lite_opts will replace for subkey_opts on some infrequently used column families
for reduce memory usage in some scenarios.

@git-hulk git-hulk requested review from git-hulk, PragmaTwice and torwig and removed request for git-hulk April 20, 2024 11:05
src/storage/storage.cc Outdated Show resolved Hide resolved
src/storage/storage.cc Outdated Show resolved Hide resolved
src/config/config.cc Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

@jjz921024 Thanks for your contributions.

@PragmaTwice
Copy link
Member

PragmaTwice commented Apr 20, 2024

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".

Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

Copy link

sonarcloud bot commented Apr 20, 2024

@jjz921024
Copy link
Contributor Author

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 reduce_infrequently_column_memory_usage so that explicitly tell the user that it is used to reduce the memory size of an infrequently used column family?

src/config/config.cc Outdated Show resolved Hide resolved
Comment on lines 349 to 352
lite_opts.write_buffer_size = 16 * KiB;
pubsub_opts.write_buffer_size = 16 * KiB;
propagate_opts.write_buffer_size = 16 * KiB;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

src/storage/storage.cc Outdated Show resolved Hide resolved
src/config/config.cc Outdated Show resolved Hide resolved
kvrocks.conf Outdated Show resolved Hide resolved
src/storage/storage.cc Outdated Show resolved Hide resolved
@PragmaTwice
Copy link
Member

PragmaTwice commented Apr 25, 2024

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 reduce_infrequently_column_memory_usage so that explicitly tell the user that it is used to reduce the memory size of an infrequently used column family?

It can be minor_columns_write_buffer_size num.

@jjz921024 jjz921024 changed the title Add a lite_mode option to reduce write_buffer_size for some column families Add a option to specify write_buffer_size for some column families Apr 25, 2024
@jjz921024 jjz921024 changed the title Add a option to specify write_buffer_size for some column families Add a option to specify write_buffer_size for minor column families Apr 25, 2024
@git-hulk
Copy link
Member

@jjz921024 I'm good with this implementation.

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice now!

src/config/config.cc Outdated Show resolved Hide resolved
@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better a default value?

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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)},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, would this conflict with rocksdb.write_buffer_size? Should we denote that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. first exec config set rocksdb.write_buffer_size num to adjust all cf
  2. and exec config set minor-columns-write-buffer-size num for 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we limit the CF set by "rocksdb.write_buffer_size"?

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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an argument of rocks_db?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

kvrocks.conf Show resolved Hide resolved
Comment on lines 349 to 350
pubsub_opts.write_buffer_size = config_->minor_columns_write_buffer_size * KiB;
propagate_opts.write_buffer_size = config_->minor_columns_write_buffer_size * KiB;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain for these two lines of code?

Copy link
Contributor Author

@jjz921024 jjz921024 Apr 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to specify write_buffer_size of all minor column families except for metadata and subkey?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think by default it can be 0, which means its value as same as rocksdb.write_buffer_size.

@mapleFU
Copy link
Member

mapleFU commented Apr 28, 2024

@jjz921024 Maybe a simpler way is:

  1. Config an extra "rocksdb.minor_write_buffer_size" as extra arguments, it can be a bit smaller
  2. "default" and "metadata" cf is "major", other column families are "minor"
  3. Adjust the size for these two parts. There are two kinds of ways: (1) using same default "rocksdb.minor_write_buffer_size", or even a smaller "rocksdb.minor_write_buffer_size", and separate the two args. This would nice for new user and most users, since it could reducing the arg here and just improve some performance (2) Using current way, this would be good for some extra user who adjusted the write_buffer_size previously

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants