You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2020/08/26 10:40:34 UTC
[GitHub] [incubator-pegasus] zhangyifan27 opened a new pull request #587: pegasus-server: Load options from file when open an exist DB
zhangyifan27 opened a new pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587
### What problem does this PR solve? <!--add issue link with summary if exists-->
#571
### What is changed and how it works?
RocksDB database will automatically persist its current set of options into a file since RocksDB 4.3[1],
so when DB exit, we should reopen the rocksdb instance using the options file stored in the db directory.
If store usage scenario into meta column family, we need to open DB with default options and then read
meta data and set options, that would also lead to unexpected flushs and compactions.
[1] https://github.com/facebook/rocksdb/wiki/RocksDB-Options-File
### Check List <!--REMOVE the items that are not applicable-->
Tests <!-- At least one of them must be included. -->
- Unit test
- Integration test
Code changes
- Has exported function/method change
Side effects
- Increased code complexity
Related changes
- Need to cherry-pick to the release branch
- Need to be included in the release note
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] neverchanje merged pull request #587: fix: Load options from file when open an exist DB
Posted by GitBox <gi...@apache.org>.
neverchanje merged pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a change in pull request #587: fix(pegasus-server): Load options from file when open an exist DB
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587#discussion_r478799945
##########
File path: src/server/pegasus_server_impl.cpp
##########
@@ -1375,7 +1414,12 @@ ::dsn::error_code pegasus_server_impl::start(int argc, char **argv)
_meta_store->set_data_version(PEGASUS_DATA_VERSION_MAX);
_meta_store->set_last_flushed_decree(0);
_meta_store->set_last_manual_compact_finish_time(0);
+ _meta_store->set_usage_scenario(ROCKSDB_ENV_USAGE_SCENARIO_NORMAL);
Review comment:
We can specify envs when create new table, so actual usage scenario may be included in envs which is passed from meta server, see https://github.com/apache/incubator-pegasus/blob/dea9756663847acb326539f61b80c6c48f92456e/src/server/pegasus_server_impl.cpp#L1256
You can read it from envs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] Shuo-Jia commented on a change in pull request #587: fix(pegasus-server): Load options from file when open an exist DB
Posted by GitBox <gi...@apache.org>.
Shuo-Jia commented on a change in pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587#discussion_r478271738
##########
File path: src/server/meta_store.cpp
##########
@@ -52,38 +52,64 @@ uint64_t meta_store::get_last_manual_compact_finish_time() const
uint64_t meta_store::get_decree_from_readonly_db(rocksdb::DB *db,
rocksdb::ColumnFamilyHandle *meta_cf) const
{
+ std::string str_last_flushed_decree;
uint64_t last_flushed_decree = 0;
- auto ec = get_value_from_meta_cf(db, meta_cf, true, LAST_FLUSHED_DECREE, &last_flushed_decree);
+ auto ec = get_string_value_from_meta_cf(
Review comment:
Why not use `get_value_from_meta_cf` directly?
##########
File path: src/server/pegasus_server_impl.cpp
##########
@@ -1375,6 +1414,7 @@ ::dsn::error_code pegasus_server_impl::start(int argc, char **argv)
_meta_store->set_data_version(PEGASUS_DATA_VERSION_MAX);
_meta_store->set_last_flushed_decree(0);
_meta_store->set_last_manual_compact_finish_time(0);
+ _meta_store->set_usage_scenario(ROCKSDB_ENV_USAGE_SCENARIO_NORMAL);
Review comment:
Defautly, the `get_envs` will show `null` result when using `shell`, if here `set_usage_scenario(ROCKSDB_ENV_USAGE_SCENARIO_NORMAL)` at first `start`, does that mean the `get_envs` will show `normal` after reopen when using `shell`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] Shuo-Jia commented on a change in pull request #587: fix(pegasus-server): Load options from file when open an exist DB
Posted by GitBox <gi...@apache.org>.
Shuo-Jia commented on a change in pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587#discussion_r478316297
##########
File path: src/server/meta_store.cpp
##########
@@ -52,38 +52,64 @@ uint64_t meta_store::get_last_manual_compact_finish_time() const
uint64_t meta_store::get_decree_from_readonly_db(rocksdb::DB *db,
rocksdb::ColumnFamilyHandle *meta_cf) const
{
+ std::string str_last_flushed_decree;
uint64_t last_flushed_decree = 0;
- auto ec = get_value_from_meta_cf(db, meta_cf, true, LAST_FLUSHED_DECREE, &last_flushed_decree);
+ auto ec = get_string_value_from_meta_cf(
Review comment:
So is `get_value_from_meta_cf` useless？
I see `get_value_from_meta_cf` has been refactor using `get_string_value_from_meta_cf` and the code is almost duplicate with here, maybe should offer `get_value_from_meta_cf` overload-version supporting pass `db` and `meta_cf`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] neverchanje commented on a change in pull request #587: fix: Load options from file when open an exist DB
Posted by GitBox <gi...@apache.org>.
neverchanje commented on a change in pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587#discussion_r478809519
##########
File path: src/server/meta_store.cpp
##########
@@ -72,18 +84,38 @@ ::dsn::error_code meta_store::get_value_from_meta_cf(rocksdb::DB *db,
uint64_t *value)
{
std::string data;
+ auto ec = get_string_value_from_meta_cf(db, cf, read_flushed_data, key, &data);
+ if (ec != ::dsn::ERR_OK) {
+ return ec;
+ }
+ dassert_f(dsn::buf2uint64(data, *value),
+ "rocksdb {} get {} from meta column family got error value {}",
Review comment:
```suggestion
"rocksdb {} get \"{}\" from meta column family failed to parse into uint64",
```
I think you cannot simply print a value out, there could be unprintable characters.
##########
File path: src/server/meta_store.cpp
##########
@@ -58,6 +58,18 @@ uint64_t meta_store::get_decree_from_readonly_db(rocksdb::DB *db,
return last_flushed_decree;
}
+std::string meta_store::get_usage_scenario() const
+{
+ // If couldn't find rocksdb usage scenario in meta column family, return normal in default.
+ std::string usage_scenario = ROCKSDB_ENV_USAGE_SCENARIO_NORMAL;
+ auto ec = get_string_value_from_meta_cf(false, ROCKSDB_ENV_USAGE_SCENARIO_KEY, &usage_scenario);
+ dassert_replica(ec == ::dsn::ERR_OK || ec == ::dsn::ERR_OBJECT_NOT_FOUND,
+ "rocksdb {} get {} from meta column family failed",
+ _db->GetName(),
+ ROCKSDB_ENV_USAGE_SCENARIO_KEY);
Review comment:
```suggestion
dassert_replica(ec == ::dsn::ERR_OK || ec == ::dsn::ERR_OBJECT_NOT_FOUND,
"rocksdb {} get {} from meta column family failed: {}",
_db->GetName(),
ROCKSDB_ENV_USAGE_SCENARIO_KEY,
ec.to_string());
```
##########
File path: src/server/pegasus_server_impl.cpp
##########
@@ -1326,22 +1327,59 @@ ::dsn::error_code pegasus_server_impl::start(int argc, char **argv)
ddebug("%s: start to open rocksDB's rdb(%s)", replica_name(), path.c_str());
+ // 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;
if (db_exist) {
- // When DB exist, meta CF must be present.
bool missing_meta_cf = true;
- if (check_meta_cf(path, &missing_meta_cf) != ::dsn::ERR_OK) {
- derror_replica("check meta column family failed");
+ bool missing_data_cf = true;
+ // Load latest options from option file stored in the db directory.
+ rocksdb::DBOptions loaded_db_opt;
+ std::vector<rocksdb::ColumnFamilyDescriptor> loaded_cf_descs;
+ rocksdb::ColumnFamilyOptions loaded_data_cf_opts;
+ // Set `ignore_unknown_options` true for forward compatibility.
+ auto status = rocksdb::LoadLatestOptions(path,
+ rocksdb::Env::Default(),
+ &loaded_db_opt,
+ &loaded_cf_descs,
+ /*ignore_unknown_options*/ true);
+ if (!status.ok()) {
+ derror_replica("load latest option file failed.");
return ::dsn::ERR_LOCAL_APP_FAILURE;
}
+ for (int i = 0; i < loaded_cf_descs.size(); ++i) {
+ if (loaded_cf_descs[i].name == META_COLUMN_FAMILY_NAME) {
+ missing_meta_cf = false;
+ } else if (loaded_cf_descs[i].name == DATA_COLUMN_FAMILY_NAME) {
+ missing_data_cf = false;
+ loaded_data_cf_opts = loaded_cf_descs[i].options;
+ } else {
+ derror_replica("unkown column family name.");
Review comment:
```suggestion
derror_replica("unknown column family name.");
```
##########
File path: src/server/pegasus_server_impl.cpp
##########
@@ -1326,22 +1327,59 @@ ::dsn::error_code pegasus_server_impl::start(int argc, char **argv)
ddebug("%s: start to open rocksDB's rdb(%s)", replica_name(), path.c_str());
+ // 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;
if (db_exist) {
- // When DB exist, meta CF must be present.
bool missing_meta_cf = true;
- if (check_meta_cf(path, &missing_meta_cf) != ::dsn::ERR_OK) {
- derror_replica("check meta column family failed");
+ bool missing_data_cf = true;
+ // Load latest options from option file stored in the db directory.
+ rocksdb::DBOptions loaded_db_opt;
+ std::vector<rocksdb::ColumnFamilyDescriptor> loaded_cf_descs;
+ rocksdb::ColumnFamilyOptions loaded_data_cf_opts;
+ // Set `ignore_unknown_options` true for forward compatibility.
+ auto status = rocksdb::LoadLatestOptions(path,
+ rocksdb::Env::Default(),
+ &loaded_db_opt,
+ &loaded_cf_descs,
+ /*ignore_unknown_options*/ true);
+ if (!status.ok()) {
+ derror_replica("load latest option file failed.");
return ::dsn::ERR_LOCAL_APP_FAILURE;
}
+ for (int i = 0; i < loaded_cf_descs.size(); ++i) {
+ if (loaded_cf_descs[i].name == META_COLUMN_FAMILY_NAME) {
+ missing_meta_cf = false;
+ } else if (loaded_cf_descs[i].name == DATA_COLUMN_FAMILY_NAME) {
+ missing_data_cf = false;
+ loaded_data_cf_opts = loaded_cf_descs[i].options;
+ } else {
+ derror_replica("unkown column family name.");
+ return ::dsn::ERR_LOCAL_APP_FAILURE;
+ }
+ }
+ // When DB exist, meta CF and default CF must be present.
dassert_replica(!missing_meta_cf, "You must upgrade Pegasus server from 2.0");
+ dassert_replica(!missing_data_cf, "Missing default column family");
Review comment:
```suggestion
// When DB exists, meta CF and data CF must be present.
dassert_replica(!missing_meta_cf, "You must upgrade Pegasus server from 2.0"
dassert_replica(!missing_data_cf, "Missing data column family");
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] Shuo-Jia commented on a change in pull request #587: fix(pegasus-server): Load options from file when open an exist DB
Posted by GitBox <gi...@apache.org>.
Shuo-Jia commented on a change in pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587#discussion_r478317976
##########
File path: src/server/pegasus_server_impl.cpp
##########
@@ -1360,6 +1398,7 @@ ::dsn::error_code pegasus_server_impl::start(int argc, char **argv)
if (db_exist) {
_last_committed_decree = _meta_store->get_last_flushed_decree();
_pegasus_data_version = _meta_store->get_data_version();
+ _usage_scenario = _meta_store->get_usage_scenario();
Review comment:
Should not, she `set_usage_scenario` at first open
https://github.com/apache/incubator-pegasus/blob/ec3b23092e4c677ac14a2ab0e7b0c3966dec5a15/src/server/pegasus_server_impl.cpp#L1417
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] zhangyifan27 commented on a change in pull request #587: fix(pegasus-server): Load options from file when open an exist DB
Posted by GitBox <gi...@apache.org>.
zhangyifan27 commented on a change in pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587#discussion_r478306484
##########
File path: src/server/meta_store.cpp
##########
@@ -52,38 +52,64 @@ uint64_t meta_store::get_last_manual_compact_finish_time() const
uint64_t meta_store::get_decree_from_readonly_db(rocksdb::DB *db,
rocksdb::ColumnFamilyHandle *meta_cf) const
{
+ std::string str_last_flushed_decree;
uint64_t last_flushed_decree = 0;
- auto ec = get_value_from_meta_cf(db, meta_cf, true, LAST_FLUSHED_DECREE, &last_flushed_decree);
+ auto ec = get_string_value_from_meta_cf(
Review comment:
`get_value_from_meta_cf` reads data from `_db`, but `get_string_value_from_meta_cf` reads data from a readonly db.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] zhangyifan27 commented on a change in pull request #587: fix(pegasus-server): Load options from file when open an exist DB
Posted by GitBox <gi...@apache.org>.
zhangyifan27 commented on a change in pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587#discussion_r478344826
##########
File path: src/server/pegasus_server_impl.cpp
##########
@@ -1360,6 +1398,7 @@ ::dsn::error_code pegasus_server_impl::start(int argc, char **argv)
if (db_exist) {
_last_committed_decree = _meta_store->get_last_flushed_decree();
_pegasus_data_version = _meta_store->get_data_version();
+ _usage_scenario = _meta_store->get_usage_scenario();
Review comment:
Indeed, maybe `get_usage_scenario()` should return `ROCKSDB_ENV_USAGE_SCENARIO_NORMAL` if we couldn't find usage_scenario in rocksdb meta column family.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on pull request #587: fix(pegasus-server): Load options from file when open an exist DB
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587#issuecomment-680980326
> If store usage scenario into meta column family, we need to open DB with default options and then read
meta data and set options, that would also lead to unexpected flushs and compactions.
Sure, usage scenario stored in meta column family is not used to initialize options to open db, but I think it's worthy to initialize member variable `_usage_scenario`, which is not well initialized currently, and it's an empty string after constructing. If restore it from meta CF, we can prevent calling `set_options` which may cause flush and compaction by https://github.com/apache/incubator-pegasus/blob/ea1689f5ca46dbdc27a15054d73e2a9a5ae0a37e/src/server/pegasus_server_impl.cpp#L2453
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] zhangyifan27 commented on a change in pull request #587: fix(pegasus-server): Load options from file when open an exist DB
Posted by GitBox <gi...@apache.org>.
zhangyifan27 commented on a change in pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587#discussion_r478341449
##########
File path: src/server/pegasus_server_impl.cpp
##########
@@ -1375,6 +1414,7 @@ ::dsn::error_code pegasus_server_impl::start(int argc, char **argv)
_meta_store->set_data_version(PEGASUS_DATA_VERSION_MAX);
_meta_store->set_last_flushed_decree(0);
_meta_store->set_last_manual_compact_finish_time(0);
+ _meta_store->set_usage_scenario(ROCKSDB_ENV_USAGE_SCENARIO_NORMAL);
Review comment:
It depends on where we read data from when using pegasus_shell `get_app_envs`.
I think we will read data from meta server, so we would still get an empty string.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] hycdong commented on a change in pull request #587: fix(pegasus-server): Load options from file when open an exist DB
Posted by GitBox <gi...@apache.org>.
hycdong commented on a change in pull request #587:
URL: https://github.com/apache/incubator-pegasus/pull/587#discussion_r478290427
##########
File path: src/server/meta_store.cpp
##########
@@ -52,38 +52,64 @@ uint64_t meta_store::get_last_manual_compact_finish_time() const
uint64_t meta_store::get_decree_from_readonly_db(rocksdb::DB *db,
rocksdb::ColumnFamilyHandle *meta_cf) const
{
+ std::string str_last_flushed_decree;
uint64_t last_flushed_decree = 0;
- auto ec = get_value_from_meta_cf(db, meta_cf, true, LAST_FLUSHED_DECREE, &last_flushed_decree);
+ auto ec = get_string_value_from_meta_cf(
+ db, meta_cf, true, LAST_FLUSHED_DECREE, &str_last_flushed_decree);
dcheck_eq_replica(::dsn::ERR_OK, ec);
+ dassert_f(dsn::buf2uint64(str_last_flushed_decree, last_flushed_decree),
Review comment:
I suggest using `dassert_replica` instead.
##########
File path: src/server/pegasus_server_impl.cpp
##########
@@ -1360,6 +1398,7 @@ ::dsn::error_code pegasus_server_impl::start(int argc, char **argv)
if (db_exist) {
_last_committed_decree = _meta_store->get_last_flushed_decree();
_pegasus_data_version = _meta_store->get_data_version();
+ _usage_scenario = _meta_store->get_usage_scenario();
Review comment:
I think it will cause core while rolling_update server. The table created before won't have `usage_scenario` in meta column family, while rolling update, the `db_exist` is true, it will assert on Line72 in function `get_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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org