You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "nathanlo99 (via GitHub)" <gi...@apache.org> on 2024/09/20 18:34:12 UTC
[PR] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
nathanlo99 opened a new pull request, #2548:
URL: https://github.com/apache/kvrocks/pull/2548
It would be nice to be able to spawn a read-only server on a snapshot of a given server.
This PR implements this feature, and introduces two new flags `--use-checkpoint` and `--keep-checkpoint` to manage the snapshot.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#issuecomment-2428111321
Would you mind also add example for how you'd like use this? ( better on doc of cli or doc of project?)
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1809726902
##########
src/config/config.cc:
##########
@@ -190,6 +188,7 @@ Config::Config() {
new IntField(&force_compact_file_min_deleted_percentage, 10, 1, 100)},
{"db-name", true, new StringField(&db_name, "change.me.db")},
{"dir", true, new StringField(&dir, kDefaultDir)},
+ {"snapshot-dir", false, new StringField(&snapshot_dir, "")},
Review Comment:
```suggestion
{"snapshot-dir", true, new StringField(&snapshot_dir, "")},
```
I think it should be read-only.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "nathanlo-hrt (via GitHub)" <gi...@apache.org>.
nathanlo-hrt commented on PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#issuecomment-2414173971
@PragmaTwice @git-hulk could you take another look at this? It seems the previous opportunity to merge was missed because new changes made it to the unstable branch.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#issuecomment-2436139169
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2548) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2548&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2548&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2548&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2548&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2548&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2548)
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#issuecomment-2460611645
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2548) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2548&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2548&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2548&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2548&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2548&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2548)
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1806741365
##########
src/cli/main.cc:
##########
@@ -170,8 +197,27 @@ int main(int argc, char *argv[]) {
}
#endif
+ const bool use_snapshot = config.snapshot_dir != "";
+ if (use_snapshot) {
+ if (const auto s = CreateSnapshot(config, config.snapshot_dir); !s.IsOK()) {
+ LOG(ERROR) << "Failed to create snapshot: " << s.Msg();
+ return 1;
+ }
+ LOG(INFO) << "Starting server in read-only mode with snapshot dir: " << config.snapshot_dir;
+ config.db_dir = config.snapshot_dir + "/db";
+ }
+ const auto snapshot_exit = MakeScopeExit([use_snapshot, &config]() {
+ if (use_snapshot && !config.keep_snapshot) {
+ LOG(INFO) << "Removing snapshot dir: " << config.snapshot_dir;
+ std::filesystem::remove_all(config.snapshot_dir);
+ }
+ });
+
engine::Storage storage(&config);
- s = storage.Open();
+ const bool read_only = use_snapshot;
+ const DBOpenMode open_mode = read_only ? kDBOpenModeForReadOnly : kDBOpenModeDefault;
+ s = storage.Open(open_mode);
Review Comment:
Since now they are just normal options, I think we'd better to move these logic to config.cc or storage.cc, to make it more maintainable.
##########
src/cli/main.cc:
##########
@@ -170,8 +197,27 @@ int main(int argc, char *argv[]) {
}
#endif
+ const bool use_snapshot = config.snapshot_dir != "";
+ if (use_snapshot) {
+ if (const auto s = CreateSnapshot(config, config.snapshot_dir); !s.IsOK()) {
+ LOG(ERROR) << "Failed to create snapshot: " << s.Msg();
+ return 1;
+ }
+ LOG(INFO) << "Starting server in read-only mode with snapshot dir: " << config.snapshot_dir;
+ config.db_dir = config.snapshot_dir + "/db";
+ }
+ const auto snapshot_exit = MakeScopeExit([use_snapshot, &config]() {
+ if (use_snapshot && !config.keep_snapshot) {
+ LOG(INFO) << "Removing snapshot dir: " << config.snapshot_dir;
+ std::filesystem::remove_all(config.snapshot_dir);
+ }
+ });
+
engine::Storage storage(&config);
- s = storage.Open();
+ const bool read_only = use_snapshot;
+ const DBOpenMode open_mode = read_only ? kDBOpenModeForReadOnly : kDBOpenModeDefault;
+ s = storage.Open(open_mode);
Review Comment:
Since now they are just normal options, I think we'd better to move these logic to config.cc or storage.cc, just like other options, to make it more maintainable.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1831997027
##########
src/cli/main.cc:
##########
@@ -171,7 +171,8 @@ int main(int argc, char *argv[]) {
#endif
engine::Storage storage(&config);
- s = storage.Open();
+ s = storage.Open(kDBOpenModeDefault);
Review Comment:
Hmm I think we don't need to change it?
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#issuecomment-2414724186
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2548) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2548&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2548&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2548&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2548&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2548&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2548)
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "nathanlo99 (via GitHub)" <gi...@apache.org>.
nathanlo99 commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1830187198
##########
src/storage/storage.cc:
##########
@@ -75,12 +76,46 @@ const int64_t kIORateLimitMaxMb = 1024000;
using rocksdb::Slice;
+static Status CreateSnapshot(Config &config, const std::string &snapshot_location) {
Review Comment:
> Do you need create it on server and consume it on cli?
I'm not sure i understand the second part of your question
> Actually I don't really understand how this works, seems this create a checkpoint when opening the file?
As for how this works, it will read the data from the `dir` passed by the user, then use RocksDB snapshot functionality to make a read-only copy of that data into the `snapshot-dir` directory, then spawn a read-only server on that copy.
This is helpful for my team to debug a running kvrocks app without accidentally changing the values of the data or closing the existing read-write server.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1806743467
##########
src/cli/main.cc:
##########
@@ -170,8 +197,27 @@ int main(int argc, char *argv[]) {
}
#endif
+ const bool use_snapshot = config.snapshot_dir != "";
+ if (use_snapshot) {
+ if (const auto s = CreateSnapshot(config, config.snapshot_dir); !s.IsOK()) {
+ LOG(ERROR) << "Failed to create snapshot: " << s.Msg();
+ return 1;
+ }
+ LOG(INFO) << "Starting server in read-only mode with snapshot dir: " << config.snapshot_dir;
+ config.db_dir = config.snapshot_dir + "/db";
+ }
+ const auto snapshot_exit = MakeScopeExit([use_snapshot, &config]() {
+ if (use_snapshot && !config.keep_snapshot) {
+ LOG(INFO) << "Removing snapshot dir: " << config.snapshot_dir;
+ std::filesystem::remove_all(config.snapshot_dir);
+ }
+ });
+
engine::Storage storage(&config);
- s = storage.Open();
+ const bool read_only = use_snapshot;
+ const DBOpenMode open_mode = read_only ? kDBOpenModeForReadOnly : kDBOpenModeDefault;
+ s = storage.Open(open_mode);
Review Comment:
Also the `CreateSnapshot` function, it need to be moved out of `cli/main.cc` since its funtionality is out of CLI scope.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1806739765
##########
src/config/config.cc:
##########
@@ -159,6 +159,8 @@ Config::Config() {
new IntField(&force_compact_file_min_deleted_percentage, 10, 1, 100)},
{"db-name", true, new StringField(&db_name, "change.me.db")},
{"dir", true, new StringField(&dir, kDefaultDir)},
+ {"snapshot-dir", false, new StringField(&snapshot_dir, "")},
+ {"keep-snapshot", false, new YesNoField(&keep_snapshot, false)},
Review Comment:
We can add them into `kvrocks.conf`, and add some comments for these new options.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1769475815
##########
src/cli/main.cc:
##########
@@ -116,6 +128,32 @@ static void InitGoogleLog(const Config *config) {
}
}
+static void CreateCheckpoint(Config &config, const std::string &checkpoint_location) {
+ // The Storage destructor deletes anything at the checkpoint_dir, so we need to make sure it's empty in case the user
+ // happens to use a checkpoint folder name which matches the default (checkpoint/)
+ const std::string old_checkpoint_dir = std::exchange(config.checkpoint_dir, "");
+ const auto checkpoint_dir_guard =
+ MakeScopeExit([&config, &old_checkpoint_dir] { config.checkpoint_dir = old_checkpoint_dir; });
+
+ engine::Storage storage(&config);
+ if (const auto s = storage.Open(kDBOpenModeForReadOnly); !s.IsOK()) {
Review Comment:
Can return the error as a Status instead of an exception.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "nathanlo99 (via GitHub)" <gi...@apache.org>.
nathanlo99 commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1830191308
##########
src/storage/storage.cc:
##########
@@ -75,12 +76,46 @@ const int64_t kIORateLimitMaxMb = 1024000;
using rocksdb::Slice;
+static Status CreateSnapshot(Config &config, const std::string &snapshot_location) {
+ // The Storage destructor deletes anything at the checkpoint_dir, so we need to make
+ // sure it's empty in case the user happens to use a snapshot name which matches the
+ // default (checkpoint/)
+ const std::string old_checkpoint_dir = std::exchange(config.checkpoint_dir, "");
+ const auto checkpoint_dir_guard =
+ MakeScopeExit([&config, &old_checkpoint_dir] { config.checkpoint_dir = old_checkpoint_dir; });
+
+ engine::Storage storage(&config);
+ if (const auto s = storage.Open(kDBOpenModeForReadOnly); !s.IsOK()) {
+ return {Status::NotOK, fmt::format("failed to open DB in read-only mode: {}", s.Msg())};
+ }
+
+ rocksdb::Checkpoint *snapshot = nullptr;
+ if (const auto s = rocksdb::Checkpoint::Create(storage.GetDB(), &snapshot); !s.ok()) {
+ return {Status::NotOK, s.ToString()};
+ }
+
+ std::unique_ptr<rocksdb::Checkpoint> snapshot_guard(snapshot);
+ if (const auto s = snapshot->CreateCheckpoint(snapshot_location + "/db"); !s.ok()) {
+ return {Status::NotOK, s.ToString()};
+ }
+
+ return Status::OK();
+}
+
Storage::Storage(Config *config)
: backup_creating_time_secs_(util::GetTimeStamp<std::chrono::seconds>()),
env_(rocksdb::Env::Default()),
config_(config),
lock_mgr_(16),
db_stats_(std::make_unique<DBStats>()) {
+ if (config->snapshot_dir != "") {
+ if (const auto s = CreateSnapshot(*config, config->snapshot_dir); !s.IsOK()) {
+ throw std::runtime_error(fmt::format("Failed to create snapshot: {}", s.Msg()));
Review Comment:
Done; had to do a bit more state management to avoid infinite recursion, since `CreateSnapshot` also calls `Storage::Open()`
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1809780034
##########
src/storage/storage.cc:
##########
@@ -75,12 +76,46 @@ const int64_t kIORateLimitMaxMb = 1024000;
using rocksdb::Slice;
+static Status CreateSnapshot(Config &config, const std::string &snapshot_location) {
Review Comment:
Actually I don't really understand how this works, seems this create a checkpoint when opening the file? Do you need create it on server and consume it on cli?
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1806741365
##########
src/cli/main.cc:
##########
@@ -170,8 +197,27 @@ int main(int argc, char *argv[]) {
}
#endif
+ const bool use_snapshot = config.snapshot_dir != "";
+ if (use_snapshot) {
+ if (const auto s = CreateSnapshot(config, config.snapshot_dir); !s.IsOK()) {
+ LOG(ERROR) << "Failed to create snapshot: " << s.Msg();
+ return 1;
+ }
+ LOG(INFO) << "Starting server in read-only mode with snapshot dir: " << config.snapshot_dir;
+ config.db_dir = config.snapshot_dir + "/db";
+ }
+ const auto snapshot_exit = MakeScopeExit([use_snapshot, &config]() {
+ if (use_snapshot && !config.keep_snapshot) {
+ LOG(INFO) << "Removing snapshot dir: " << config.snapshot_dir;
+ std::filesystem::remove_all(config.snapshot_dir);
+ }
+ });
+
engine::Storage storage(&config);
- s = storage.Open();
+ const bool read_only = use_snapshot;
+ const DBOpenMode open_mode = read_only ? kDBOpenModeForReadOnly : kDBOpenModeDefault;
+ s = storage.Open(open_mode);
Review Comment:
Since now they are just normal options, I think we'd better to move these logic to config.cc or storage.cc, just like other options, to make it more maintainable.
WDYT?
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1769471472
##########
src/cli/main.cc:
##########
@@ -46,6 +48,8 @@
#include "version_util.h"
Server *srv = nullptr;
+std::optional<std::string> checkpoint_location = std::nullopt;
+bool keep_checkpoint = false;
Review Comment:
We can move them to CliOptions rather than global variables.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1769471472
##########
src/cli/main.cc:
##########
@@ -46,6 +48,8 @@
#include "version_util.h"
Server *srv = nullptr;
+std::optional<std::string> checkpoint_location = std::nullopt;
+bool keep_checkpoint = false;
Review Comment:
We can move them to CLIOptions rather than global variables.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "nathanlo99 (via GitHub)" <gi...@apache.org>.
nathanlo99 commented on PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#issuecomment-2460265643
apologies for the linter back and forth: i'm unable to build this locally with the same set-up as the linter uses, so a lot of these issues fall through the cracks
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "nathanlo99 (via GitHub)" <gi...@apache.org>.
nathanlo99 commented on PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#issuecomment-2474564754
@mapleFU I'm not sure how to edit the docs, but one example workflow is the following.
Start a read-write Kvrocks server writing data to `~/kvrocks-dir`, for the purposes of writing important data.
```
$ kvrocks [...] --dir ~/kvrocks-dir
```
Say the user realizes some of the important data is incorrect: they'd like to debug their script and run it on a copy of the data, possibly changing it. They'd like to do this without fear of corrupting or changing the original server's copy of the data:
```
$ mkdir ~/debug-dir
$ kvrocks [...] --dir ~/kvrocks-dir --snapshot-dir ~/debug-dir
```
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "nathanlo99 (via GitHub)" <gi...@apache.org>.
nathanlo99 commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1830187198
##########
src/storage/storage.cc:
##########
@@ -75,12 +76,46 @@ const int64_t kIORateLimitMaxMb = 1024000;
using rocksdb::Slice;
+static Status CreateSnapshot(Config &config, const std::string &snapshot_location) {
Review Comment:
> Do you need create it on server and consume it on cli?
I'm not sure i understand the second part of your question
> Actually I don't really understand how this works, seems this create a checkpoint when opening the file?
As for how this works, it will read the data from the `dir` passed by the user, then use RocksDB snapshot functionality to make a read-only copy of that data into the `snapshot-dir` directory, then spawn a read-only server on that copy.
This is helpful for my team to debug a running kvrocks app without accidentally changing the values of the data or closing the existing read-write server.
##########
src/storage/storage.cc:
##########
@@ -75,12 +76,46 @@ const int64_t kIORateLimitMaxMb = 1024000;
using rocksdb::Slice;
+static Status CreateSnapshot(Config &config, const std::string &snapshot_location) {
Review Comment:
> Do you need create it on server and consume it on cli?
I'm not sure i understand the second part of your question
> Actually I don't really understand how this works, seems this create a checkpoint when opening the file?
As for how this works, it will read the data from the `dir` passed by the user, then use RocksDB snapshot functionality to make a read-only copy of that data into the `snapshot-dir` directory, then spawn a read-only server on that copy.
This is helpful for my team to debug a running kvrocks app without accidentally changing the values of the data or closing the existing read-write server.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1809733457
##########
src/storage/storage.cc:
##########
@@ -75,12 +76,46 @@ const int64_t kIORateLimitMaxMb = 1024000;
using rocksdb::Slice;
+static Status CreateSnapshot(Config &config, const std::string &snapshot_location) {
+ // The Storage destructor deletes anything at the checkpoint_dir, so we need to make
+ // sure it's empty in case the user happens to use a snapshot name which matches the
+ // default (checkpoint/)
+ const std::string old_checkpoint_dir = std::exchange(config.checkpoint_dir, "");
+ const auto checkpoint_dir_guard =
+ MakeScopeExit([&config, &old_checkpoint_dir] { config.checkpoint_dir = old_checkpoint_dir; });
+
+ engine::Storage storage(&config);
+ if (const auto s = storage.Open(kDBOpenModeForReadOnly); !s.IsOK()) {
+ return {Status::NotOK, fmt::format("failed to open DB in read-only mode: {}", s.Msg())};
+ }
+
+ rocksdb::Checkpoint *snapshot = nullptr;
+ if (const auto s = rocksdb::Checkpoint::Create(storage.GetDB(), &snapshot); !s.ok()) {
+ return {Status::NotOK, s.ToString()};
+ }
+
+ std::unique_ptr<rocksdb::Checkpoint> snapshot_guard(snapshot);
+ if (const auto s = snapshot->CreateCheckpoint(snapshot_location + "/db"); !s.ok()) {
+ return {Status::NotOK, s.ToString()};
+ }
+
+ return Status::OK();
+}
+
Storage::Storage(Config *config)
: backup_creating_time_secs_(util::GetTimeStamp<std::chrono::seconds>()),
env_(rocksdb::Env::Default()),
config_(config),
lock_mgr_(16),
db_stats_(std::make_unique<DBStats>()) {
+ if (config->snapshot_dir != "") {
+ if (const auto s = CreateSnapshot(*config, config->snapshot_dir); !s.IsOK()) {
+ throw std::runtime_error(fmt::format("Failed to create snapshot: {}", s.Msg()));
Review Comment:
Hmm I think we need to move them to Storage::Open, also the throw can be replaced then.
--
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] feat: Add the ability to checkpoint an existing server, and spawn a read-only server on that view. [kvrocks]
Posted by "nathanlo99 (via GitHub)" <gi...@apache.org>.
nathanlo99 commented on code in PR #2548:
URL: https://github.com/apache/kvrocks/pull/2548#discussion_r1809529145
##########
src/cli/main.cc:
##########
@@ -170,8 +197,27 @@ int main(int argc, char *argv[]) {
}
#endif
+ const bool use_snapshot = config.snapshot_dir != "";
+ if (use_snapshot) {
+ if (const auto s = CreateSnapshot(config, config.snapshot_dir); !s.IsOK()) {
+ LOG(ERROR) << "Failed to create snapshot: " << s.Msg();
+ return 1;
+ }
+ LOG(INFO) << "Starting server in read-only mode with snapshot dir: " << config.snapshot_dir;
+ config.db_dir = config.snapshot_dir + "/db";
+ }
+ const auto snapshot_exit = MakeScopeExit([use_snapshot, &config]() {
+ if (use_snapshot && !config.keep_snapshot) {
+ LOG(INFO) << "Removing snapshot dir: " << config.snapshot_dir;
+ std::filesystem::remove_all(config.snapshot_dir);
+ }
+ });
+
engine::Storage storage(&config);
- s = storage.Open();
+ const bool read_only = use_snapshot;
+ const DBOpenMode open_mode = read_only ? kDBOpenModeForReadOnly : kDBOpenModeDefault;
+ s = storage.Open(open_mode);
Review Comment:
Thanks, this makes sense: this is done now
--
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