acelyc111 commented on code in PR #1108:
URL: https://github.com/apache/incubator-pegasus/pull/1108#discussion_r944175767
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -1560,9 +1560,10 @@ dsn::error_code pegasus_server_impl::start(int argc,
char **argv)
ddebug_replica("start to open rocksDB's rdb({})", rdb_path);
- // Here we create a `tmp_data_cf_opts` because we don't want to modify
`_data_cf_opts`, which
+ // Here we create a `_tmp_data_cf_opts` because we don't want to modify
`_data_cf_opts`, which
// will be used elsewhere.
- rocksdb::ColumnFamilyOptions tmp_data_cf_opts = _data_cf_opts;
+ _tmp_data_cf_opts = _data_cf_opts;
Review Comment:
'tmp' is not good name for a member variable.
hosw about rename '_data_cf_opts' to '_base_data_cf_opts', and rename
'_tmp_data_cf_opts' to '_table_data_cf_opts' ?
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2634,6 +2635,10 @@ void pegasus_server_impl::update_usage_scenario(const
std::map<std::string, std:
old_usage_scenario,
new_usage_scenario);
}
+ } else {
+ // When an old db is opened and the conf is changed, the options
related to usage scenario
+ // need to be recalculated with new values.
+ recalculate_data_cf_options(_tmp_data_cf_opts);
Review Comment:
If I didn't miss any thing, I found this function will not be called when
open an exist db, right?
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -3007,6 +3013,97 @@ void pegasus_server_impl::reset_usage_scenario_options(
target_opts->max_write_buffer_number = base_opts.max_write_buffer_number;
}
+void pegasus_server_impl::recalculate_data_cf_options(
+ const rocksdb::ColumnFamilyOptions &cur_data_cf_opts)
+{
+#define UPDATE_OPTION_IF_NEEDED(option, value, str_value)
\
+ if ((value) != cur_data_cf_opts.option) {
\
+ new_options[#option] = (str_value);
\
+ }
Review Comment:
How about:
```
#define UPDATE_OPTION_IF_NEEDED(option, new_value) \
if (_data_cf_opts.option != cur_data_cf_opts.option) { \
new_options[#option] = #new_value; \
}
```
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -3007,6 +3013,97 @@ void pegasus_server_impl::reset_usage_scenario_options(
target_opts->max_write_buffer_number = base_opts.max_write_buffer_number;
}
+void pegasus_server_impl::recalculate_data_cf_options(
+ const rocksdb::ColumnFamilyOptions &cur_data_cf_opts)
+{
+#define UPDATE_OPTION_IF_NEEDED(option, value, str_value)
\
+ if ((value) != cur_data_cf_opts.option) {
\
+ new_options[#option] = (str_value);
\
+ }
+
+ if (!_is_need_update_data_cf_opts)
Review Comment:
What's the aim of this variable?
--
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: dev-unsubscr...@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@pegasus.apache.org
For additional commands, e-mail: dev-h...@pegasus.apache.org
