hycdong commented on code in PR #1511:
URL:
https://github.com/apache/incubator-pegasus/pull/1511#discussion_r1227430538
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2625,6 +2626,61 @@ pegasus_server_impl::get_restore_dir_from_env(const
std::map<std::string, std::s
return res;
}
+void pegasus_server_impl::update_rocksdb_dynamic_options(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0)
+ return;
+
+ std::unordered_map<std::string, std::string> new_options;
+ for (auto &option : ROCKSDB_DYNAMIC_OPTIONS) {
+ auto find = envs.find(option);
+ if (option.compare(ROCKSDB_WRITE_BUFFER_SIZE) == 0 && find !=
envs.end()) {
+ new_options["write_buffer_size"] = find->second;
+ }
+ }
+
+ // doing set option
+ if (new_options.size() != 0 && !set_options(new_options)) {
+ LOG_WARNING("Set options fails");
Review Comment:
I suggest that you should add more information in this log, such as why set
option failed and which option is.
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2625,6 +2626,61 @@ pegasus_server_impl::get_restore_dir_from_env(const
std::map<std::string, std::s
return res;
}
+void pegasus_server_impl::update_rocksdb_dynamic_options(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0)
+ return;
+
+ std::unordered_map<std::string, std::string> new_options;
+ for (auto &option : ROCKSDB_DYNAMIC_OPTIONS) {
Review Comment:
const auto
##########
src/common/replica_envs.h:
##########
@@ -64,6 +65,12 @@ class replica_envs
static const std::string USER_SPECIFIED_COMPACTION;
static const std::string ROCKSDB_ALLOW_INGEST_BEHIND;
static const std::string UPDATE_MAX_REPLICA_COUNT;
+ static const std::string ROCKSDB_WRITE_BUFFER_SIZE;
+ static const std::string ROCKSDB_NUM_LEVELS;
+ static const std::string VALUE_VERSION;
Review Comment:
What is the usage of this variety? It seems useless.
##########
src/server/pegasus_server_impl.h:
##########
@@ -328,6 +329,11 @@ class pegasus_server_impl : public pegasus_read_service
void update_user_specified_compaction(const std::map<std::string,
std::string> &envs);
+ void update_rocksdb_dynamic_options(const std::map<std::string,
std::string> &envs);
+
+ void
+ update_rocksdb_options_before_create_replica(const std::map<std::string,
std::string> &envs);
Review Comment:
I suggest that rename this function as
`set_rocksdb_options_before_creating`. `Updating` usually means that you update
an old value into a new one, however, your caller function shows that there are
not old value existed, it actually an seting operation.
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2625,6 +2626,61 @@ pegasus_server_impl::get_restore_dir_from_env(const
std::map<std::string, std::s
return res;
}
+void pegasus_server_impl::update_rocksdb_dynamic_options(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0)
+ return;
+
+ std::unordered_map<std::string, std::string> new_options;
+ for (auto &option : ROCKSDB_DYNAMIC_OPTIONS) {
+ auto find = envs.find(option);
+ if (option.compare(ROCKSDB_WRITE_BUFFER_SIZE) == 0 && find !=
envs.end()) {
+ new_options["write_buffer_size"] = find->second;
+ }
+ }
+
+ // doing set option
+ if (new_options.size() != 0 && !set_options(new_options)) {
+ LOG_WARNING("Set options fails");
+ }
+}
+
+void pegasus_server_impl::update_rocksdb_options_before_create_replica(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0)
+ return;
+
+ for (auto &option : pegasus::ROCKSDB_STATIC_OPTIONS) {
Review Comment:
const auto
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2625,6 +2626,61 @@ pegasus_server_impl::get_restore_dir_from_env(const
std::map<std::string, std::s
return res;
}
+void pegasus_server_impl::update_rocksdb_dynamic_options(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0)
+ return;
+
+ std::unordered_map<std::string, std::string> new_options;
+ for (auto &option : ROCKSDB_DYNAMIC_OPTIONS) {
+ auto find = envs.find(option);
+ if (option.compare(ROCKSDB_WRITE_BUFFER_SIZE) == 0 && find !=
envs.end()) {
+ new_options["write_buffer_size"] = find->second;
+ }
+ }
+
+ // doing set option
+ if (new_options.size() != 0 && !set_options(new_options)) {
+ LOG_WARNING("Set options fails");
+ }
+}
+
+void pegasus_server_impl::update_rocksdb_options_before_create_replica(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0)
+ return;
+
+ for (auto &option : pegasus::ROCKSDB_STATIC_OPTIONS) {
+ auto find = envs.find(option);
+ bool is_set = false;
+ if (option.compare(ROCKSDB_NUM_LEVELS) == 0 && find != envs.end()) {
+ int32_t val = 0;
+ if (!dsn::buf2int32(find->second, val))
+ continue;
+ is_set = true;
+ _data_cf_opts.num_levels = val;
+ }
+
+ if (is_set)
+ LOG_INFO("Reset {} \"{}\" succeed", find->first, find->second);
+ }
+
+ for (auto &option : pegasus::ROCKSDB_DYNAMIC_OPTIONS) {
Review Comment:
const auto
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2625,6 +2626,61 @@ pegasus_server_impl::get_restore_dir_from_env(const
std::map<std::string, std::s
return res;
}
+void pegasus_server_impl::update_rocksdb_dynamic_options(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0)
+ return;
+
+ std::unordered_map<std::string, std::string> new_options;
+ for (auto &option : ROCKSDB_DYNAMIC_OPTIONS) {
+ auto find = envs.find(option);
+ if (option.compare(ROCKSDB_WRITE_BUFFER_SIZE) == 0 && find !=
envs.end()) {
+ new_options["write_buffer_size"] = find->second;
+ }
+ }
+
+ // doing set option
+ if (new_options.size() != 0 && !set_options(new_options)) {
+ LOG_WARNING("Set options fails");
+ }
+}
+
+void pegasus_server_impl::update_rocksdb_options_before_create_replica(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0)
+ return;
+
+ for (auto &option : pegasus::ROCKSDB_STATIC_OPTIONS) {
+ auto find = envs.find(option);
+ bool is_set = false;
+ if (option.compare(ROCKSDB_NUM_LEVELS) == 0 && find != envs.end()) {
+ int32_t val = 0;
+ if (!dsn::buf2int32(find->second, val))
+ continue;
+ is_set = true;
+ _data_cf_opts.num_levels = val;
+ }
+
+ if (is_set)
+ LOG_INFO("Reset {} \"{}\" succeed", find->first, find->second);
+ }
+
+ for (auto &option : pegasus::ROCKSDB_DYNAMIC_OPTIONS) {
+ auto find = envs.find(option);
+ bool is_set = false;
+ if (option.compare(ROCKSDB_WRITE_BUFFER_SIZE) == 0 && find !=
envs.end()) {
+ uint64_t val = 0;
+ if (!dsn::buf2uint64(find->second, val))
+ continue;
+ _data_cf_opts.write_buffer_size = static_cast<size_t>(val);
+ }
+ if (is_set)
Review Comment:
It seems that `is_set` will always be false.
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2625,6 +2626,61 @@ pegasus_server_impl::get_restore_dir_from_env(const
std::map<std::string, std::s
return res;
}
+void pegasus_server_impl::update_rocksdb_dynamic_options(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0)
+ return;
+
+ std::unordered_map<std::string, std::string> new_options;
+ for (auto &option : ROCKSDB_DYNAMIC_OPTIONS) {
+ auto find = envs.find(option);
+ if (option.compare(ROCKSDB_WRITE_BUFFER_SIZE) == 0 && find !=
envs.end()) {
+ new_options["write_buffer_size"] = find->second;
+ }
+ }
+
+ // doing set option
+ if (new_options.size() != 0 && !set_options(new_options)) {
+ LOG_WARNING("Set options fails");
+ }
+}
+
+void pegasus_server_impl::update_rocksdb_options_before_create_replica(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0)
+ return;
+
+ for (auto &option : pegasus::ROCKSDB_STATIC_OPTIONS) {
+ auto find = envs.find(option);
+ bool is_set = false;
+ if (option.compare(ROCKSDB_NUM_LEVELS) == 0 && find != envs.end()) {
+ int32_t val = 0;
+ if (!dsn::buf2int32(find->second, val))
+ continue;
+ is_set = true;
+ _data_cf_opts.num_levels = val;
+ }
+
+ if (is_set)
+ LOG_INFO("Reset {} \"{}\" succeed", find->first, find->second);
Review Comment:
I suggest using `set` rather than `reset` in this log.
##########
src/meta/app_env_validator.cpp:
##########
@@ -151,6 +174,36 @@ bool check_bool_value(const std::string &env_value,
std::string &hint_message)
return true;
}
+bool check_rocksdb_write_buffer_size(const std::string &env_value, std::string
&hint_message)
+{
+ uint64_t val = 0;
+
+ if (!dsn::buf2uint64(env_value, val)) {
+ hint_message = fmt::format("rocksdb.write_buffer_size cannot set this
val: {}", env_value);
+ return false;
+ }
+ if (val < (32 << 20) || val > (512 << 20)) {
+ hint_message =
+ fmt::format("rocksdb.write_buffer_size suggest set val in range
[33554432, 536870912]");
+ return false;
+ }
+ return true;
+}
+bool check_rocksdb_num_levels(const std::string &env_value, std::string
&hint_message)
+{
+ int32_t val = 0;
+
+ if (!dsn::buf2int32(env_value, val)) {
+ hint_message = fmt::format("rocksdb.num_levels cannot set this val:",
env_value);
Review Comment:
fmt::format("rocksdb.num_levels cannot set this val: {}", env_value)
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -3054,6 +3112,7 @@ void pegasus_server_impl::reset_usage_scenario_options(
target_opts->max_compaction_bytes = base_opts.max_compaction_bytes;
target_opts->write_buffer_size = base_opts.write_buffer_size;
target_opts->max_write_buffer_number = base_opts.max_write_buffer_number;
+ target_opts->num_levels = base_opts.num_levels;
Review Comment:
Why would you reset `num_levels` options here? `num_levels` seems won't be
modified while updating `usage_scenario`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
