[SYS-6179] Add MultiReadAsync file API. #290

dpetrov4 · 2023-10-13T17:41:26Z

Extending the file API to provide an option to implement asynchronous
multi read operation. The default implementation delegates to AsyncRead.
Updated the AsyncFileReader class implementation to use MultiReadAsync.

igorcanadi

I'm accepting this PR, but at the same time want to discuss an alternative. This PR reaches fairly deep into RocksDB and will introduce merge conflicts as we try to keep up with upstream. What do you think about leaving RocksDB as is and introducing request batching at our layer?

I'm thinking ReadAsync implementation on our end could wait for a very short period of time, and then batch up any requests that come along in that wait period. That way we might be able to even batch requests from multiple threads if we get lucky. What do you think?

file/random_access_file_reader.cc

dpetrov4 · 2023-11-28T19:17:55Z

@igorcanadi

I am not a fan of splitting multi part entities and then trying to reassemble them later, this always seem to cause some kind of tricky issue on top of being inefficient. However, this is just my personal bias -- TAO did something similar in recent years with DB missies and was quite successful at it.
The other side of the story is that I suspect that FB people working on RocksDB will see "the light" and will probably add their version of AsyncMultiGet. I suspect that didn't do it from the beginning because error handling gets tricky.

Anyways, for my taste I would go with what we have here and suffer the merge conflicts if they show up, however I don't have any experience of actually suffering through them, so LMK if you feel I am misguided here.

igorcanadi · 2023-11-29T11:57:51Z

@igorcanadi

I am not a fan of splitting multi part entities and then trying to reassemble them later, this always seem to cause some kind of tricky issue on top of being inefficient. However, this is just my personal bias -- TAO did something similar in recent years with DB missies and was quite successful at it. The other side of the story is that I suspect that FB people working on RocksDB will see "the light" and will probably add their version of AsyncMultiGet. I suspect that didn't do it from the beginning because error handling gets tricky.

Anyways, for my taste I would go with what we have here and suffer the merge conflicts if they show up, however I don't have any experience of actually suffering through them, so LMK if you feel I am misguided here.

Sounds good, let's ship this.

igorcanadi · 2024-02-13T15:07:40Z

db/version_set.cc

@@ -5683,7 +5683,7 @@ Status VersionSet::LogAndApplyHelper(ColumnFamilyData* cfd,
  }

  if (edit->HasReplicationSequence()) {
-      replication_sequence_ = edit->GetReplicationSequence();
+    replication_sequence_ = edit->GetReplicationSequence();


This might conflict with @seckcoder 's changes, and since it's only a formatting change, can we revert it pls?

igorcanadi · 2024-02-13T15:08:33Z

file/random_access_file_reader.cc

+  std::function<void(const FSReadRequest*, size_t, void*)> cb_;
+  void* cb_arg_;
+  uint64_t start_time_;
+#ifndef ROCKSDB_LITE


ROCKSDB_LITE is removed in 8.0, which I plan to upgrade to very soon, so let's remove the guard here.

igorcanadi · 2024-02-13T15:08:52Z

file/random_access_file_reader.cc

+    } else if (!req.status.IsAborted()) {
+      RecordTick(stats_, ASYNC_READ_ERROR_COUNT, 1);
+    }
+#ifndef ROCKSDB_LITE


igorcanadi · 2024-02-13T15:10:30Z

include/rocksdb/cloud/cloud_file_system.h

+  // Returns CloudManifest file name for a given db.
+  virtual std::string CloudManifestFile(const std::string& dbname) = 0;
+
+  virtual CloudManifest* GetCloudManifest() = 0;
+
+  // TODO(wei): this function is used to temporarily support open db and switch
+  // cookie. Change it to use the cookie from options once migration complete.
+  virtual IOStatus CreateCloudManifest(const std::string& local_dbname,
+                                       const std::string& cookie) = 0;
+
+  virtual IOStatus LoadCloudManifest(const std::string& local_dbname,
+                                     bool read_only) = 0;
+
+  virtual IOStatus UploadCloudManifest(const std::string& local_dbname,
+                               const std::string& cookie) const = 0;
+
+  // Prepare a local directory for use as a clone of the cloud storage
+  virtual IOStatus SanitizeLocalDirectory(const DBOptions& options,
+                                          const std::string& local_name,
+                                          bool read_only) = 0;


Are these changes unrelated?

This is related to cleaning up the APIs so that we can implement DisaggFileSystem in Rockset.

igorcanadi · 2024-02-13T15:12:21Z

include/rocksdb/file_system.h

+  virtual IOStatus Poll(std::vector<void*>& io_handles,
                        size_t /*min_completions*/) {
+    // RocksDB-Cloud contribution begin 
+    if (!io_handles.empty()) {
+      // Default implementations on async operations do not return any 
+      // io_handles. If you override them, you need to override Poll as well.
+      return IOStatus::NotSupported("Poll");
+    }
+    // RocksDB-Cloud contribution end 


Is this just for added safety? I prefer less safety if that means less merge conflicts in the future :)

igorcanadi · 2024-02-13T15:12:44Z

include/rocksdb/file_system.h

+  //
+  // When the read request is completed, callback function specified in cb
+  // should be called with arguments cb_arg and the result populated in
+  // FSReadRequest with result and status fileds updated by FileSystem.


s/fileds/fields

Extending the file API to provide an option to implement asynchronous multi read operation. The default implementation delegates to AsyncRead. Updated the AsyncFileReader class implementation to use MultiReadAsync.

Expanidng the CloudFileSystem API to include methods for handling CloudManifest. This allows us to better hide CloudFileSystemImpl and allow different implementations of CloudFileSystem.

dpetrov4 force-pushed the dmitri_multi_async branch 2 times, most recently from 86c1e00 to 3e0c7de Compare October 13, 2023 21:07

dpetrov4 marked this pull request as ready for review October 18, 2023 19:24

dpetrov4 requested review from nbronson, igorcanadi and a team October 18, 2023 19:25

dpetrov4 force-pushed the dmitri_multi_async branch from 3e0c7de to 89ef005 Compare October 18, 2023 23:08

dpetrov4 force-pushed the dmitri_multi_async branch from 89ef005 to 0e5cdb9 Compare November 13, 2023 21:34

igorcanadi approved these changes Nov 22, 2023

View reviewed changes

file/random_access_file_reader.cc Outdated Show resolved Hide resolved

dpetrov4 force-pushed the dmitri_multi_async branch from 0e5cdb9 to 114bfa1 Compare November 28, 2023 19:03

dpetrov4 force-pushed the dmitri_multi_async branch from 2351d5d to 79e24b3 Compare December 5, 2023 22:32

dpetrov4 force-pushed the dmitri_multi_async branch 6 times, most recently from 1b9a4fa to 6792b57 Compare December 20, 2023 14:50

dpetrov4 force-pushed the dmitri_multi_async branch 4 times, most recently from 1d6d20f to 6700da7 Compare December 22, 2023 17:28

dpetrov4 force-pushed the dmitri_multi_async branch 4 times, most recently from f9284f3 to a1b8d6e Compare January 21, 2024 19:00

dpetrov4 force-pushed the dmitri_multi_async branch from a1b8d6e to d1af15c Compare February 1, 2024 23:58

Detaching CloudFileSystemEnv from CloudFileSystem class hierarchy.

6f72b95

dpetrov4 force-pushed the dmitri_multi_async branch from d1af15c to 5bddc59 Compare February 12, 2024 23:25

igorcanadi approved these changes Feb 13, 2024

View reviewed changes

dpetrov4 added 3 commits February 13, 2024 21:54

[SYS-6179] Add MultiReadAsync file API.

270962a

Extending the file API to provide an option to implement asynchronous multi read operation. The default implementation delegates to AsyncRead. Updated the AsyncFileReader class implementation to use MultiReadAsync.

Switch to sync default implementation of AsyncMultiRead.

7842c84

Add CloudManifest handling API to CloudFileSystem.

2d5d260

Expanidng the CloudFileSystem API to include methods for handling CloudManifest. This allows us to better hide CloudFileSystemImpl and allow different implementations of CloudFileSystem.

dpetrov4 force-pushed the dmitri_multi_async branch from 5bddc59 to 2d5d260 Compare February 13, 2024 22:00

igorcanadi approved these changes Feb 14, 2024

View reviewed changes

dpetrov4 merged commit 61223e2 into master Feb 14, 2024
0 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYS-6179] Add MultiReadAsync file API. #290

[SYS-6179] Add MultiReadAsync file API. #290

dpetrov4 commented Oct 13, 2023

igorcanadi left a comment

dpetrov4 commented Nov 28, 2023

igorcanadi commented Nov 29, 2023

igorcanadi Feb 13, 2024

igorcanadi Feb 13, 2024

igorcanadi Feb 13, 2024

igorcanadi Feb 13, 2024

dpetrov4 Feb 13, 2024

igorcanadi Feb 13, 2024

dpetrov4 Feb 13, 2024

igorcanadi Feb 13, 2024

[SYS-6179] Add MultiReadAsync file API. #290

[SYS-6179] Add MultiReadAsync file API. #290

Conversation

dpetrov4 commented Oct 13, 2023

igorcanadi left a comment

Choose a reason for hiding this comment

dpetrov4 commented Nov 28, 2023

igorcanadi commented Nov 29, 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