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
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions kvrocks.conf
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,13 @@ json-max-nesting-depth 1024
# Default: json
json-storage-format json

# The write buffer size of minor column families those infrequently be used
# including pubsub, propagte, zset_score, stream and search.
# It will reduce the memory usage in some scenarios.
jjz921024 marked this conversation as resolved.
Show resolved Hide resolved
#
# Default: 65536 KB (same with rocksdb.write_buffer_size)
minor-columns-write-buffer-size 65536

################################## TLS ###################################

# By default, TLS/SSL is disabled, i.e. `tls-port` is set to 0.
Expand Down
15 changes: 15 additions & 0 deletions src/config/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include "server/server.h"
#include "status.h"
#include "storage/redis_metadata.h"
#include "storage/storage.h"

constexpr const char *kDefaultDir = "/tmp/kvrocks";
constexpr const char *kDefaultBackupDir = "/tmp/kvrocks/backup";
Expand Down Expand Up @@ -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?


/* rocksdb options */
{"rocksdb.compression", false,
Expand Down Expand Up @@ -575,6 +577,19 @@ void Config::initFieldCallback() {
if (!srv) return Status::OK();
return srv->GetNamespace()->LoadAndRewrite();
}},
{"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"?

kColumnFamilyIDPropagate, kColumnFamilyIDStream,
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?

if (!s.IsOK()) return s;
}
return Status::OK();
}},

{"rocksdb.target_file_size_base",
[this](Server *srv, const std::string &k, const std::string &v) -> Status {
Expand Down
1 change: 1 addition & 0 deletions src/config/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -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?

std::vector<std::string> binds;
std::string dir;
std::string db_dir;
Expand Down
18 changes: 15 additions & 3 deletions src/storage/storage.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include <random>

#include "compact_filter.h"
#include "config/config.h"
#include "db_util.h"
#include "event_listener.h"
#include "event_util.h"
Expand Down Expand Up @@ -226,6 +227,12 @@ Status Storage::SetOptionForAllColumnFamilies(const std::string &key, const std:
return Status::OK();
}

Status Storage::SetOptionForColumnFamily(ColumnFamilyID id, const std::string &key, const std::string &value) {
auto s = db_->SetOptions(GetCFHandle(id), {{key, value}});
if (!s.ok()) return {Status::NotOK, s.ToString()};
return Status::OK();
}

Status Storage::SetDBOption(const std::string &key, const std::string &value) {
auto s = db_->SetDBOptions({{key, value}});
if (!s.ok()) return {Status::NotOK, s.ToString()};
Expand Down Expand Up @@ -337,15 +344,20 @@ Status Storage::Open(DBOpenMode mode) {
propagate_opts.disable_auto_compactions = config_->rocks_db.disable_auto_compactions;
SetBlobDB(&propagate_opts);

rocksdb::ColumnFamilyOptions minor_opts(subkey_opts);
minor_opts.write_buffer_size = config_->minor_columns_write_buffer_size * KiB;
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?


std::vector<rocksdb::ColumnFamilyDescriptor> column_families;
// Caution: don't change the order of column family, or the handle will be mismatched
column_families.emplace_back(rocksdb::kDefaultColumnFamilyName, subkey_opts);
column_families.emplace_back(kMetadataColumnFamilyName, metadata_opts);
column_families.emplace_back(kZSetScoreColumnFamilyName, subkey_opts);
column_families.emplace_back(kZSetScoreColumnFamilyName, minor_opts);
column_families.emplace_back(kPubSubColumnFamilyName, pubsub_opts);
column_families.emplace_back(kPropagateColumnFamilyName, propagate_opts);
column_families.emplace_back(kStreamColumnFamilyName, subkey_opts);
column_families.emplace_back(kSearchColumnFamilyName, subkey_opts);
column_families.emplace_back(kStreamColumnFamilyName, minor_opts);
column_families.emplace_back(kSearchColumnFamilyName, minor_opts);

std::vector<std::string> old_column_families;
auto s = rocksdb::DB::ListColumnFamilies(options, config_->db_dir, &old_column_families);
Expand Down
1 change: 1 addition & 0 deletions src/storage/storage.h
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ class Storage {
void SetBlobDB(rocksdb::ColumnFamilyOptions *cf_options);
rocksdb::Options InitRocksDBOptions();
Status SetOptionForAllColumnFamilies(const std::string &key, const std::string &value);
Status SetOptionForColumnFamily(ColumnFamilyID id, const std::string &key, const std::string &value);
Status SetDBOption(const std::string &key, const std::string &value);
Status CreateColumnFamilies(const rocksdb::Options &options);
// The sequence_number will be pointed to the value of the sequence number in range of DB,
Expand Down