empiredan commented on code in PR #1511:
URL:
https://github.com/apache/incubator-pegasus/pull/1511#discussion_r1233756096
##########
src/meta/app_env_validator.cpp:
##########
@@ -151,6 +172,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)) {
Review Comment:
I think setting `write_buffer_size` to some value less than 32MB could be
still accepted, especially when the available physical memory is small ? On our
many production environment with small memory capacity it's practically set to
~20MB since Pegasus also has to share the small memory capacity with other
services.
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2625,6 +2626,79 @@ 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;
+ }
Review Comment:
```suggestion
if (envs.empty()) {
return;
}
```
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2625,6 +2626,79 @@ 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;
+ }
+ auto extract_option = [](const std::string &option) -> std::string {
+ std::stringstream ss(option);
+ std::string prefix, rocksdb_opt;
+ std::getline(ss, prefix, '.');
+ std::getline(ss, rocksdb_opt);
+ LOG_INFO("Extract rocksdb dynamic opt ({}) from ({})", rocksdb_opt,
option);
+ return rocksdb_opt;
+ };
+
+ std::unordered_map<std::string, std::string> new_options;
+ for (const auto &option : ROCKSDB_DYNAMIC_OPTIONS) {
+ auto find = envs.find(option);
+ if (find == envs.end()) {
+ continue;
+ }
+ new_options[extract_option(option)] = find->second;
+ }
+
+ // doing set option
+ if (new_options.empty() && set_options(new_options)) {
+ LOG_INFO("Set rocksdb dynamic options success");
+ }
+}
+
+void pegasus_server_impl::set_rocksdb_options_before_creating(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0) {
+ return;
+ }
+
+ for (const auto &option : pegasus::ROCKSDB_STATIC_OPTIONS) {
+ auto find = envs.find(option);
+ if (find == envs.end()) {
+ continue;
+ }
+ bool is_set = false;
+ if (option.compare(ROCKSDB_NUM_LEVELS) == 0) {
Review Comment:
I think we could implement a setter for each option:
```c++
using cf_opts_setter = std::function<bool(const std::string &,
rocksdb::ColumnFamilyOptions &)>;
std::unordered_map<std::string, cf_opts_setter> kCfOptsSetters = {...};
```
This `setter` encapsulate all logic including string-to-int converter such
as `buf2*`, and returns a `boolean` representing if the string is valid. We
could also introduce a new map `kCfOptsSetters` as above , or just as a value
of `ROCKSDB_DYNAMIC_OPTIONS` defined in `pegasus_const.cpp`.
##########
src/meta/server_state.cpp:
##########
@@ -1151,6 +1151,9 @@ void server_state::create_app(dsn::message_ex *msg)
!validate_target_max_replica_count(request.options.replica_count)) {
response.err = ERR_INVALID_PARAMETERS;
will_create_app = false;
+ } else if (!validate_app_envs(request.options.envs)) {
Review Comment:
Could you please add some test cases for
[meta_app_operation_test/create_app](https://github.com/apache/incubator-pegasus/blob/master/src/meta/test/meta_app_operation_test.cpp#L325)
?
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2625,6 +2626,79 @@ 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;
+ }
+ auto extract_option = [](const std::string &option) -> std::string {
+ std::stringstream ss(option);
+ std::string prefix, rocksdb_opt;
+ std::getline(ss, prefix, '.');
+ std::getline(ss, rocksdb_opt);
+ LOG_INFO("Extract rocksdb dynamic opt ({}) from ({})", rocksdb_opt,
option);
+ return rocksdb_opt;
+ };
+
+ std::unordered_map<std::string, std::string> new_options;
+ for (const auto &option : ROCKSDB_DYNAMIC_OPTIONS) {
+ auto find = envs.find(option);
+ if (find == envs.end()) {
+ continue;
+ }
+ new_options[extract_option(option)] = find->second;
+ }
+
+ // doing set option
+ if (new_options.empty() && set_options(new_options)) {
Review Comment:
Add some test cases to verify if the options are really set into rocksdb ?
```suggestion
if (!new_options.empty() && set_options(new_options)) {
```
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2625,6 +2626,79 @@ 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;
+ }
+ auto extract_option = [](const std::string &option) -> std::string {
+ std::stringstream ss(option);
+ std::string prefix, rocksdb_opt;
+ std::getline(ss, prefix, '.');
+ std::getline(ss, rocksdb_opt);
+ LOG_INFO("Extract rocksdb dynamic opt ({}) from ({})", rocksdb_opt,
option);
+ return rocksdb_opt;
+ };
+
+ std::unordered_map<std::string, std::string> new_options;
+ for (const auto &option : ROCKSDB_DYNAMIC_OPTIONS) {
+ auto find = envs.find(option);
+ if (find == envs.end()) {
+ continue;
+ }
+ new_options[extract_option(option)] = find->second;
+ }
+
+ // doing set option
+ if (new_options.empty() && set_options(new_options)) {
+ LOG_INFO("Set rocksdb dynamic options success");
+ }
+}
+
+void pegasus_server_impl::set_rocksdb_options_before_creating(
+ const std::map<std::string, std::string> &envs)
+{
+ if (envs.size() == 0) {
+ return;
+ }
Review Comment:
```suggestion
if (envs.empty()) {
return;
}
```
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2625,6 +2626,79 @@ 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;
+ }
+ auto extract_option = [](const std::string &option) -> std::string {
+ std::stringstream ss(option);
+ std::string prefix, rocksdb_opt;
+ std::getline(ss, prefix, '.');
+ std::getline(ss, rocksdb_opt);
+ LOG_INFO("Extract rocksdb dynamic opt ({}) from ({})", rocksdb_opt,
option);
+ return rocksdb_opt;
+ };
+
+ std::unordered_map<std::string, std::string> new_options;
+ for (const auto &option : ROCKSDB_DYNAMIC_OPTIONS) {
+ auto find = envs.find(option);
+ if (find == envs.end()) {
+ continue;
+ }
+ new_options[extract_option(option)] = find->second;
Review Comment:
Consider using `dsn::utils::split_args` to extract the second field ?
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -3038,6 +3114,22 @@ bool pegasus_server_impl::set_usage_scenario(const
std::string &usage_scenario)
}
}
+void pegasus_server_impl::reset_rocksdb_options(const
rocksdb::ColumnFamilyOptions &base_opts,
+ rocksdb::ColumnFamilyOptions
*target_opts)
+{
+ // Reset rocksdb option includes two aspects:
+ // 1. Set usage_scenario related rocksdb options
+ // 2. Rocksdb option set in app envs, consists of ROCKSDB_DYNAMIC_OPTIONS
and
Review Comment:
Later, once some new dynamic or static options are introduced, how do we
guarantee that the new introduced options would not be forgotten to be added
into this function ?
--
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]
