You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "chrisxu333 (via GitHub)" <gi...@apache.org> on 2024/01/15 00:32:26 UTC
[PR] persist cluster-enabled status in RocksDB [kvrocks]
chrisxu333 opened a new pull request, #2020:
URL: https://github.com/apache/kvrocks/pull/2020
To close #1410 .
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "chrisxu333 (via GitHub)" <gi...@apache.org>.
chrisxu333 commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1452454699
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::overrideClusterMode() {
Review Comment:
I think `checkClusterMode` would be better. Changed accordingly.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1452108413
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::overrideClusterMode() {
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled =
+ storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+
+ if (check_cluster_enabled.IsNotFound()) {
+ return storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
+ }
+ if (check_cluster_enabled.ok()) {
+ if (config_->cluster_enabled != stoi(value)) {
+ LOG(ERROR) << "cluster_enable status from config file is inconsistent with the persisted one";
+ return {Status::NotOK, "mismatching cluster_enable status"};
Review Comment:
```suggestion
return {Status::NotOK, "mismatched cluster_enabled status"};
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1452111243
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::overrideClusterMode() {
Review Comment:
Could you give it a better name? IMHO "override" is not appropriate here.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#issuecomment-1891311782
@chrisxu333 Please, run `./x.py format` to format the codebase and CI will be happy :)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#issuecomment-1892315536
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
No data about Coverage
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2020&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1452085492
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::forceClusterMode() {
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled =
+ storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+
+ if (check_cluster_enabled.IsNotFound()) {
+ Status s = storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
Review Comment:
Use `yes` or `no` as the value in rocksdb instead of `0` or `1` with to_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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451890263
##########
src/server/server.cc:
##########
@@ -152,6 +152,19 @@ Status Server::Start() {
}
}
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled = storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+ if(check_cluster_enabled.IsNotFound()) {
+ s = storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
+ if(!s.IsOK()) return s;
+ } else if(check_cluster_enabled.ok()) {
+ // use the one that's previously persisted.
+ config_->cluster_enabled = std::stoi(value);
+ } else {
+ return {Status::NotOK, "failed reading from storage"};
+ }
Review Comment:
Could you move these lines to a separate 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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "chrisxu333 (via GitHub)" <gi...@apache.org>.
chrisxu333 commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451972192
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::forceClusterMode() {
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled =
+ storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+
+ if (check_cluster_enabled.IsNotFound()) {
+ Status s = storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
Review Comment:
Could you elaborate more on this?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "chrisxu333 (via GitHub)" <gi...@apache.org>.
chrisxu333 commented on PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#issuecomment-1892667202
Hi @git-hulk could you also help diagnose why test is failed? It was successful when I did locally.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#issuecomment-1891341122
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[2 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
No data about Coverage
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2020&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1452109089
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::overrideClusterMode() {
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled =
+ storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+
+ if (check_cluster_enabled.IsNotFound()) {
+ return storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
+ }
+ if (check_cluster_enabled.ok()) {
+ if (config_->cluster_enabled != stoi(value)) {
+ LOG(ERROR) << "cluster_enable status from config file is inconsistent with the persisted one";
Review Comment:
```suggestion
LOG(ERROR) << "cluster_enabled status from config file is inconsistent with the persisted one";
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#issuecomment-1891134307
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[2 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
No data about Coverage
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2020&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451853065
##########
src/server/server.cc:
##########
@@ -152,6 +152,19 @@ Status Server::Start() {
}
}
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled = storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+ if(check_cluster_enabled.IsNotFound()) {
+ s = storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
+ if(!s.IsOK()) return s;
+ } else if(check_cluster_enabled.ok()) {
+ // use the one that's previously persisted.
+ config_->cluster_enabled = std::stoi(value);
Review Comment:
In this line, I suggest printing a warning log if the persistence is inconsistent with the configuration file.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1452108004
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::overrideClusterMode() {
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled =
+ storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+
+ if (check_cluster_enabled.IsNotFound()) {
+ return storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
+ }
+ if (check_cluster_enabled.ok()) {
+ if (config_->cluster_enabled != stoi(value)) {
Review Comment:
Please use `ParseInt` in `parse_utl.h` instead of `stoi` here to avoid exception throwing.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1590349076
##########
src/server/server.cc:
##########
@@ -152,6 +152,11 @@ Status Server::Start() {
}
}
+ s = checkClusterMode();
Review Comment:
Seems this should be before the `namespace_.LoadAndRewrite()`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1452479085
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,27 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::checkClusterMode() {
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled =
+ storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+
+ if (check_cluster_enabled.IsNotFound()) {
+ return storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
+ }
+ if (check_cluster_enabled.ok()) {
+ auto is_enabled = GET_OR_RET(ParseInt<int64_t>(value, 10));
+ if (config_->cluster_enabled != is_enabled) {
+ LOG(ERROR) << "cluster_enabled status from config file is inconsistent with the persisted one";
+ return {Status::NotOK, "mismatching cluster_enabled status"};
Review Comment:
```suggestion
return {Status::NotOK, "inconsistent cluster_enabled status"};
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#issuecomment-1892344774
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
No data about Coverage
[0.7% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2020&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#issuecomment-1893032526
> Hi @git-hulk could you also help diagnose why the newly introduced testcase is failed? It was successful when I did locally.
perhaps you can have a try at debug yourself, and I will also take a look while I'm free.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1590349076
##########
src/server/server.cc:
##########
@@ -152,6 +152,11 @@ Status Server::Start() {
}
}
+ s = checkClusterMode();
Review Comment:
Seems this should be before the `namespace_.LoadAndRewrite()`. To avoid some weird error.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451890511
##########
src/server/server.cc:
##########
@@ -152,6 +152,19 @@ Status Server::Start() {
}
}
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled = storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+ if(check_cluster_enabled.IsNotFound()) {
+ s = storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
+ if(!s.IsOK()) return s;
+ } else if(check_cluster_enabled.ok()) {
+ // use the one that's previously persisted.
+ config_->cluster_enabled = std::stoi(value);
Review Comment:
I prefer a fatal error rather than a warning.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451977359
##########
kvrocks.conf:
##########
@@ -43,8 +43,11 @@ daemonize no
# If you enable cluster, kvrocks will encode key with its slot id calculated by
# CRC16 and modulo 16384, encoding key with its slot id makes it efficient to
# migrate keys based on the slot. So if you enabled at first time, cluster mode must
-# not be disabled after restarting, and vice versa. That is to say, data is not
-# compatible between standalone mode with cluster mode, you must migrate data
+# not be disabled after restarting, and vice versa. Currently, kvrocks will keep
+# using the cluster-enabled status that is persisted at first time, regardless of
+# what cluster-enabled status is provided afterwards.
Review Comment:
Me too, I also tend to just throw an error that prevents the user from starting the server. This is equivalent to adding a quick pre check.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1452235671
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::overrideClusterMode() {
Review Comment:
How about checkClusterMode or validateClusterMode if it throws an error once the cluster mode is mismatched between the db and the configuration file?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451889991
##########
src/storage/storage.h:
##########
@@ -80,6 +80,8 @@ constexpr const char *kLuaFuncSHAPrefix = "lua_f_";
constexpr const char *kLuaFuncLibPrefix = "lua_func_lib_";
constexpr const char *kLuaLibCodePrefix = "lua_lib_code_";
+const std::string kClusterEnabledKey = "cluster_enabled";
Review Comment:
```suggestion
const std::string kClusterEnabledKey = "config_cluster_enabled";
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451963120
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::forceClusterMode() {
Review Comment:
Is overrideClusterMode better than forceClusterMode?
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::forceClusterMode() {
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled =
+ storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+
+ if (check_cluster_enabled.IsNotFound()) {
+ Status s = storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
+ if (!s.IsOK()) return s;
+ } else {
+ if (check_cluster_enabled.ok()) {
+ LOG(WARNING) << "cluster_enable status is inconsistent. Using the previously persisted one.";
Review Comment:
Need to compare the configuration between the config file and rocksdb? or it might confuse users.
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::forceClusterMode() {
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled =
+ storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+
+ if (check_cluster_enabled.IsNotFound()) {
+ Status s = storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
+ if (!s.IsOK()) return s;
Review Comment:
Can return the status directly if the key is not found
```suggestion
return storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
```
##########
src/server/server.cc:
##########
@@ -1839,6 +1844,26 @@ void Server::cleanupExitedWorkerThreads(bool force) {
}
}
+Status Server::forceClusterMode() {
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled =
+ storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+
+ if (check_cluster_enabled.IsNotFound()) {
+ Status s = storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
Review Comment:
I prefer using `yes` or `no` instead of the bool value.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451969408
##########
kvrocks.conf:
##########
@@ -43,8 +43,11 @@ daemonize no
# If you enable cluster, kvrocks will encode key with its slot id calculated by
# CRC16 and modulo 16384, encoding key with its slot id makes it efficient to
# migrate keys based on the slot. So if you enabled at first time, cluster mode must
-# not be disabled after restarting, and vice versa. That is to say, data is not
-# compatible between standalone mode with cluster mode, you must migrate data
+# not be disabled after restarting, and vice versa. Currently, kvrocks will keep
+# using the cluster-enabled status that is persisted at first time, regardless of
+# what cluster-enabled status is provided afterwards.
Review Comment:
force rewrite or can not start the server? HDYT @git-hulk @PragmaTwice
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "chrisxu333 (via GitHub)" <gi...@apache.org>.
chrisxu333 commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451976258
##########
kvrocks.conf:
##########
@@ -43,8 +43,11 @@ daemonize no
# If you enable cluster, kvrocks will encode key with its slot id calculated by
# CRC16 and modulo 16384, encoding key with its slot id makes it efficient to
# migrate keys based on the slot. So if you enabled at first time, cluster mode must
-# not be disabled after restarting, and vice versa. That is to say, data is not
-# compatible between standalone mode with cluster mode, you must migrate data
+# not be disabled after restarting, and vice versa. Currently, kvrocks will keep
+# using the cluster-enabled status that is persisted at first time, regardless of
+# what cluster-enabled status is provided afterwards.
Review Comment:
Okay I'll change accordingly. By throwing an error you mean also to prevent the user from starting the server on mismatch right?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451980351
##########
kvrocks.conf:
##########
@@ -43,8 +43,11 @@ daemonize no
# If you enable cluster, kvrocks will encode key with its slot id calculated by
# CRC16 and modulo 16384, encoding key with its slot id makes it efficient to
# migrate keys based on the slot. So if you enabled at first time, cluster mode must
-# not be disabled after restarting, and vice versa. That is to say, data is not
-# compatible between standalone mode with cluster mode, you must migrate data
+# not be disabled after restarting, and vice versa. Currently, kvrocks will keep
+# using the cluster-enabled status that is persisted at first time, regardless of
+# what cluster-enabled status is provided afterwards.
Review Comment:
> By throwing an error you mean also to prevent the user from starting the server on mismatch right?
That's right.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "chrisxu333 (via GitHub)" <gi...@apache.org>.
chrisxu333 commented on PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#issuecomment-1891133589
@git-hulk Please help review :)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451853065
##########
src/server/server.cc:
##########
@@ -152,6 +152,19 @@ Status Server::Start() {
}
}
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled = storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+ if(check_cluster_enabled.IsNotFound()) {
+ s = storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
+ if(!s.IsOK()) return s;
+ } else if(check_cluster_enabled.ok()) {
+ // use the one that's previously persisted.
+ config_->cluster_enabled = std::stoi(value);
Review Comment:
In this line, I suggest printing a log warning that the persistence is inconsistent with the configuration file.
##########
src/server/server.cc:
##########
@@ -152,6 +152,19 @@ Status Server::Start() {
}
}
+ std::string value;
+ auto cf = storage->GetCFHandle(engine::kPropagateColumnFamilyName);
+ rocksdb::Status check_cluster_enabled = storage->Get(rocksdb::ReadOptions(), cf, rocksdb::Slice(engine::kClusterEnabledKey), &value);
+ if(check_cluster_enabled.IsNotFound()) {
+ s = storage->WriteToPropagateCF(engine::kClusterEnabledKey, std::to_string(config_->cluster_enabled));
+ if(!s.IsOK()) return s;
+ } else if(check_cluster_enabled.ok()) {
+ // use the one that's previously persisted.
+ config_->cluster_enabled = std::stoi(value);
+ } else {
+ return {Status::NotOK, "failed reading from storage"};
Review Comment:
"failed to load cluster_enable from storage: "+ check_cluster_enabled.ToString()
##########
src/server/server.cc:
##########
@@ -152,6 +152,19 @@ Status Server::Start() {
}
}
+ std::string value;
Review Comment:
We'd better clarify this behavior in the configuration file:
https://github.com/apache/kvrocks/blob/9d656f1476583651e2fae20ed1c37cf35d0679ca/kvrocks.conf#L46-L48
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#discussion_r1451974687
##########
kvrocks.conf:
##########
@@ -43,8 +43,11 @@ daemonize no
# If you enable cluster, kvrocks will encode key with its slot id calculated by
# CRC16 and modulo 16384, encoding key with its slot id makes it efficient to
# migrate keys based on the slot. So if you enabled at first time, cluster mode must
-# not be disabled after restarting, and vice versa. That is to say, data is not
-# compatible between standalone mode with cluster mode, you must migrate data
+# not be disabled after restarting, and vice versa. Currently, kvrocks will keep
+# using the cluster-enabled status that is persisted at first time, regardless of
+# what cluster-enabled status is provided afterwards.
Review Comment:
Both of them are good for me. I prefer throwing an error if we have to choose one.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#issuecomment-1891595558
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
No data about Coverage
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2020&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#issuecomment-1891905243
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
No data about Coverage
[0.7% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2020&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] persist cluster-enabled status in RocksDB [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2020:
URL: https://github.com/apache/kvrocks/pull/2020#issuecomment-1892338482
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2020&resolved=false&inNewCodePeriod=true)
No data about Coverage
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2020&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2020)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org