feat: Implement rdb snapshot write directly into s3. #1205

romange · 2023-05-11T04:57:23Z

Load flow reorganized - most of the logic is now in InferLoadFile function. S3 read is not yet supported.
Write path is implemented. Specifically, you can use undocumented (by design) option to save like: SAVE rdb s3://bucket/path/file.
When using --dir=s3://bucket/path/ it also saves into s3 on shutdown.

src/server/server_family.cc

src/server/server_family.h

chakaz · 2023-05-11T09:30:38Z

src/server/server_family.cc

+}
+
+// Returns bucket_name, obj_path for an s3 path.
+optional<pair<string_view, string_view>> GetBucketPath(string_view path) {


Given this is an internal function (side note: maybe wrap it in an anonymous namespace?), I'd define a struct for its return type, i.e. struct AwsData { string_view bucket_name; string_view obj_path; };

BTW consider using ABSL_ATTRIBUTE_LIFETIME_BOUND (see here) to indicate that the returned value cannot surpass the input's lifetime

yes, it's already anynymous.

Oppah, welcome to crust++! Gladly, but I did not quite understand where I should put it to mark a return value as bound to the parameter.

I think it should be right after the argument, as in:

optional<pair<string_view, string_view>> GetBucketPath(string_view path ABSL_ATTRIBUTE_LIFETIME_BOUND)

However I'm not sure, it's fairly new and I have not yet used it myself. Does it work as such?

it works but I did not like how much visual clutter it adds. Decided to just return strings.

chakaz · 2023-05-11T09:34:25Z

src/server/server_family.cc

+      error_code file_ec;
+      data_folder = fs::canonical(dir, file_ec);
+      if (file_ec) {
+        LOG(ERROR) << "Data directory error: " << file_ec.message();


Consider including dir in the LOG()

chakaz · 2023-05-11T09:41:47Z

src/server/server_family.cc

+    }
+
+    fs::path fl_path{obj_path};
+    fl_path /= dbname;


Nit: I'm not a big fan of using operator /= for appending paths (I'd use .append() instead), but that's very much a matter of preference

hh, np, changed.

chakaz · 2023-05-11T09:43:05Z

src/server/server_family.cc

@@ -204,9 +269,16 @@ class RdbSnapshot {
    return started_ || (saver_ && saver_->Mode() == SaveMode::SUMMARY);
  }

+  void SetAWS(cloud::AWS* aws) {
+    aws_ = aws;


I guess this is a non-owning pointer, perhaps add a documentation saying such?

chakaz · 2023-05-11T09:48:36Z

src/server/server_family.cc

@@ -279,7 +374,9 @@ void ExtendDfsFilenameWithShard(int shard, fs::path* filename) {
 }

 GenericError ValidateFilename(const fs::path& filename, bool new_version) {
-  if (!filename.parent_path().empty()) {
+  bool has_cloud_prefix = absl::StartsWith(filename.c_str(), "s3://");


Why not use IsCloudPath() here?

chakaz · 2023-05-11T09:50:49Z

src/server/server_family.cc

-    data_folder = fs::canonical(dir, file_ec);
+  string flag_dir = GetFlag(FLAGS_dir);
+  if (IsCloudPath(flag_dir)) {
+    aws_.reset(new cloud::AWS("s3"));


It's safer to use std::make_unique<cloud::AWS>("s3") instead of .reset(). Generally, calling reset() and unique_ptr's c'tor directly is considered bad practice.

src/server/server_family.cc

chakaz · 2023-05-12T07:46:36Z

I'm still new to GitHub, but I don't see the updated code? Am I doing something wrong?

1. Load flow reorganized - most of the logic is now in InferLoadFile function. S3 read is not yet supported. 2. Write path is implemented. Specifically, you can use undocumented (by design) option to save like: `SAVE rdb s3://bucket/path/file`. 3. When using `--dir=s3://bucket/path/` it also saves into s3 on shutdown. Signed-off-by: Roman Gershman <roman@dragonflydb.io>

romange · 2023-05-12T16:33:45Z

You should see the new diff here: https://github.com/dragonflydb/dragonfly/pull/1205/files
for example, I can see I return pair<string, string> in there

1. Load flow reorganized - most of the logic is now in InferLoadFile function. S3 read is not yet supported. 2. Write path is implemented. Specifically, you can use undocumented (by design) option to save like: `SAVE rdb s3://bucket/path/file`. 3. When using `--dir=s3://bucket/path/` it also saves into s3 on shutdown. Signed-off-by: Roman Gershman <roman@dragonflydb.io>

romange requested a review from chakaz May 11, 2023 04:57

chakaz reviewed May 11, 2023

View reviewed changes

romange force-pushed the S3Write branch from 5e6bbac to 46fa142 Compare May 11, 2023 20:56

romange requested a review from chakaz May 11, 2023 20:56

romange force-pushed the S3Write branch from 46fa142 to d8b6b05 Compare May 12, 2023 11:27

chakaz approved these changes May 12, 2023

View reviewed changes

romange merged commit 4e96c56 into main May 13, 2023
6 checks passed

romange deleted the S3Write branch May 13, 2023 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement rdb snapshot write directly into s3. #1205

feat: Implement rdb snapshot write directly into s3. #1205

romange commented May 11, 2023

chakaz May 11, 2023

romange May 11, 2023

chakaz May 12, 2023

romange May 12, 2023

chakaz May 11, 2023

chakaz May 11, 2023

romange May 11, 2023

chakaz May 11, 2023

chakaz May 11, 2023

chakaz May 11, 2023

chakaz commented May 12, 2023

romange commented May 12, 2023 •

edited

feat: Implement rdb snapshot write directly into s3. #1205

feat: Implement rdb snapshot write directly into s3. #1205

Conversation

romange commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chakaz commented May 12, 2023

romange commented May 12, 2023 • edited

romange commented May 12, 2023 •

edited