Read from blob cache first when MultiGetBlob() #10225

gangliao · 2022-06-21T22:12:41Z

Summary:

There is currently no caching mechanism for blobs, which is not ideal especially when the database resides on remote storage (where we cannot rely on the OS page cache). As part of this task, we would like to make it possible for the application to configure a blob cache.

Test Plan:

Add test cases for MultiGetBlob

Tasks:

In this task, we added the new API MultiGetBlob() for BlobSource.

This PR is a part of #10156

db/blob/blob_source.cc

ltamasi · 2022-06-21T23:02:02Z

db/blob/blob_source.cc

+  size_t num_blobs = user_keys.size();
+  assert(num_blobs > 0);
+  assert(num_blobs <= MultiGetContext::MAX_BATCH_SIZE);
+  assert(num_blobs == offsets.size());
+  assert(num_blobs == value_sizes.size());
+  assert(num_blobs == statuses.size());
+  assert(num_blobs == blobs.size());


We could consider passing a single autovector of structs instead of a separate one for each of these

// A read Blob request structure for use in MultiGetBlob struct ReadRequest { // User key to lookup the paired blob const Slice* user_key; // File offset in bytes uint64_t offset; // Length to read in bytes size_t len; // Output parameter set by MultiGetBlob() to point to the data buffer, and // the number of valid bytes PinnableSlice* result; // Status of read Status* status; }; std::array<ReadRequest, num_blobs> request_buf; autovector<ReadRequest*> requests;

Sounds like we can't do this in VersionSet's MultiGetBlob() since num_blobs is not statically available.

Right, std::array requires a size that's available at compile time. You could use an std::vector or, even better, an autovector though.

P.S. Not sure about the need for request_buf. Any reason autovector<ReadRequest> requests would not work?

autovector<ReadRequest> requests looks good. We need to copy one more time when calling file_reader's MultiGetBlob (I believe we also need to change its MultiGetBlob). But, the cost of the copy is accepted.

This change will also need to modify Version, we can do it in the same new PR. Added: #10156 (comment)

BTW, the name BlobReadRequest already be used by @riversand963. Let me know if you have a better name for it.

using BlobReadRequest = std::pair<BlobIndex, std::reference_wrapper<const KeyContext>>; using BlobReadRequests = std::vector<BlobReadRequest>; void MultiGetBlob(const ReadOptions& read_options, MultiGetRange& range, std::unordered_map<uint64_t, BlobReadRequests>& blob_rqs);

Feel free to rename the types if the proposal is better

ltamasi · 2022-06-21T23:09:25Z

db/blob/blob_source.cc

@@ -152,6 +153,140 @@ Status BlobSource::GetBlob(const ReadOptions& read_options,
  return s;
 }

+void BlobSource::MultiGetBlob(


I'm wondering if it would make sense to push more of the logic into BlobSource, like the logic that sorts blobs by offset for example.

That makes sense to me. Since I also need to modify 'VersionSet`'s MultiGetBlob and add new end-to-end test cases, we can put them in a new PR.

Added to my to-do list.

Added: #10156 (comment)

db/blob/blob_source.cc

riversand963 · 2022-06-22T17:01:07Z

db/blob/blob_source.cc

+void BlobSource::MultiGetBlob(
+    const ReadOptions& read_options,
+    const autovector<std::reference_wrapper<const Slice>>& user_keys,
+    uint64_t file_number, uint64_t file_size,


I assume we will support MultiGetBlob() from multiple files?

I think MultiGetBlob() is to get multiple blobs from same file. For multiple files, we have an upper level function for that in VersionSet:

rocksdb/db/version_set.cc

Line 1931 in a16e2ff

const uint64_t blob_file_number = elem.first;

Version::MultiGetBlob will call BlobSource::MultiGetBlob multiple times.

That's actually a very good question, shall we put Version::MultiGetBlob's logic into a new BlobSource::MultiGetBlob?

Since on the Version layer, we already support reading multiple blobs from multiple files in MultiGetBlob(), I think we should continue to support this.
How do you think of the following

Version::MultiGetBlob(...) // multiple files multiple blobs -> Version::MultiGetBlobFromOneFile() -> BlobSource::MultiGetBlob() // one file, multiple blobs

or

Version::MultiGetBlob(...) // multiple files multiple blobs -> BlobSource::MultiGetBlob() // multiple files multiple blobs -> BlobSource::MultiGetBlobFromOneFile() // one file, multiple blobs

Seems the latter is better since, by definition, BlobSource also has information about multiple blob files, thus we can push the logic into this layer.

+1 for this. I think this is another piece that could be pushed down to BlobSource. (Well, most of it I guess: it would make sense to keep e.g. the blob file number validity check in Version.)

EDIT: I think @riversand963 's second suggestion would be the way to go

All right. Let's make a PR for cleanup version and add new api MultiGetBlobFromOneFile. I don't want to do another huge PR.

Added: #10156 (comment)

db/blob/blob_source.cc

db/blob/blob_source_test.cc

facebook-github-bot · 2022-06-22T20:58:12Z

@gangliao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ltamasi

LGTM, thanks! Let's make those changes we discussed in the next PR

ltamasi · 2022-06-23T20:25:44Z

db/blob/blob_source.cc

+      s = GetBlobFromCache(key, &blob_entry);
+      if (s.ok() && blob_entry.GetValue()) {
+        assert(statuses[i]);
+        assert(blob_entry.GetValue()->size() == value_sizes[i]);


Just for the record, we'll probably have to adjust/remove this assertion (as discussed, it doesn't hold for compressed blobs)

Summary: Update HISTORY.md for blob cache. Implementation can be found from Github issue #10156 (or Github PRs #10155, #10178, #10225, #10198, and #10272). Pull Request resolved: #10328 Reviewed By: riversand963 Differential Revision: D37732514 Pulled By: gangliao fbshipit-source-id: 4c942a41c07914bfc8db56a0d3cf4d3e53d5963f

rebase codebase

d256160

facebook-github-bot added the CLA Signed label Jun 21, 2022

gangliao commented Jun 21, 2022

View reviewed changes

db/blob/blob_source.cc Show resolved Hide resolved

gangliao mentioned this pull request Jun 21, 2022

Read from blob cache first when MultiGetBlob() #10199

Closed

Fix windows warning

cb7caca

gangliao requested review from ltamasi, riversand963 and akankshamahajan15 June 21, 2022 23:20

gangliao mentioned this pull request Jun 21, 2022

BlobDB Caching #10156

Closed

14 tasks

ltamasi reviewed Jun 21, 2022

View reviewed changes

Fix naming convention and typo

3136869

riversand963 reviewed Jun 22, 2022

View reviewed changes

Cover the failure of blob file reader creation

0e22802

gangliao commented Jun 22, 2022

View reviewed changes

db/blob/blob_source_test.cc Show resolved Hide resolved

gangliao commented Jun 22, 2022

View reviewed changes

db/blob/blob_source_test.cc Show resolved Hide resolved

Set status only when cache hit

1883913

ltamasi approved these changes Jun 23, 2022

View reviewed changes

facebook-github-bot closed this in ba1f62d Jun 23, 2022

gangliao mentioned this pull request Jul 8, 2022

Update HISTORY.md for blob cache #10328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read from blob cache first when MultiGetBlob() #10225

Read from blob cache first when MultiGetBlob() #10225

gangliao commented Jun 21, 2022

ltamasi Jun 21, 2022

gangliao Jun 22, 2022 •

edited by riversand963

Loading

ltamasi Jun 22, 2022 •

edited

Loading

gangliao Jun 22, 2022

gangliao Jun 22, 2022

gangliao Jun 22, 2022

riversand963 Jun 22, 2022

ltamasi Jun 21, 2022

gangliao Jun 21, 2022 •

edited

Loading

gangliao Jun 22, 2022

riversand963 Jun 22, 2022

gangliao Jun 22, 2022

gangliao Jun 22, 2022 •

edited

Loading

riversand963 Jun 22, 2022

ltamasi Jun 22, 2022 •

edited

Loading

gangliao Jun 22, 2022

gangliao Jun 22, 2022

facebook-github-bot commented Jun 22, 2022

ltamasi left a comment

ltamasi Jun 23, 2022

Read from blob cache first when MultiGetBlob() #10225

Read from blob cache first when MultiGetBlob() #10225

Conversation

gangliao commented Jun 21, 2022

Choose a reason for hiding this comment

gangliao Jun 22, 2022 • edited by riversand963 Loading

Choose a reason for hiding this comment

ltamasi Jun 22, 2022 • edited Loading

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

gangliao Jun 21, 2022 • edited Loading

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

gangliao Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltamasi Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 22, 2022

ltamasi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gangliao Jun 22, 2022 •

edited by riversand963

Loading

ltamasi Jun 22, 2022 •

edited

Loading

gangliao Jun 21, 2022 •

edited

Loading

gangliao Jun 22, 2022 •

edited

Loading

ltamasi Jun 22, 2022 •

edited

Loading