Fix BlobDB::Get which only get out the value offset

[foolenough]

Blob db use StackableDB::get which only get out the
value offset, but not the value.
Fix by making BlobDB::Get override the designated getter.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[yiwu-arbug]

@foolenough Thank you for the fix! Do you mind signing the CLA?

[foolenough]

@yiwu-arbug I've signed the CLA now. Thanks for your review.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[yiwu-arbug]

@foolenough Thanks. Actually I don't quite understand the fix. It seems that we forget to update BlobDB::Get with PinnableSlice, but how does it cause blob db return offset rather than value? And it doesn't seems to be caught by blob_db_test.

[facebook-github-bot]

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[foolenough]

@yiwu-arbug There are three version of Get method in db.h

// db.h
virtual inline Status Get(const ReadOptions& options,
                           ColumnFamilyHandle* column_family, const Slice& key,
                           std::string* value) {
   assert(value != nullptr);
   PinnableSlice pinnable_val(value);
   assert(!pinnable_val.IsPinned());
   auto s = Get(options, column_family, key, &pinnable_val);
   if (s.ok() && pinnable_val.IsPinned()) {
     value->assign(pinnable_val.data(), pinnable_val.size());
   }  // else value is already assigned
   return s;
 }
 virtual Status Get(const ReadOptions& options,
                    ColumnFamilyHandle* column_family, const Slice& key,
                    PinnableSlice* value) = 0;
 virtual Status Get(const ReadOptions& options, const Slice& key, std::string* value) {
   return Get(options, DefaultColumnFamily(), key, value);
 }

The StackableDB override the designated getter of DB correctly.

// Stackable
using DB::Get;
  virtual Status Get(const ReadOptions& options,
                     ColumnFamilyHandle* column_family, const Slice& key,
                     PinnableSlice* value) override {
    return db_->Get(options, column_family, key, value);
  }

But the BlobDB::Get override the wrong version of getter.

// blob_db.h
virtual Status Get(const ReadOptions& options,
                 ColumnFamilyHandle* column_family, const Slice& key,
                 std::string* value)

Thus the Get(const ReadOptions& options, ColumnFamilyHandle* column_family, const Slice& key, PinnableSlice* value) is not overrided, and it would call the StackableDB::Get, which just return the value offset but not the actual value.

The blob_db_test calls blobdb->Get(const ReadOptions& options, ColumnFamilyHandle* column_family, const Slice& key, std::string* value), which is correctly override. But this method is not the designated getter, the getter methods is not consistent with each other.

When run benchmark with db_bench readrandom -use_blob_db, which use PinnableSlice version getter, the result is wrongly too high, as it doesn't fetch the value in blob file.

[facebook-github-bot]

@foolenough updated the pull request - view changes - changes since last import

[yiwu-arbug]

I see. Do you mind adding a test to blob_db_test, which wrap BlobDB with a StackableDB and verify return value of StackableDB::Get is correct?

[yiwu-arbug]

BTW are you trying to use blob db? The feature is not ready yet. There are bug fixing ongoing.

[facebook-github-bot]

@foolenough updated the pull request - view changes - changes since last import

[facebook-github-bot]

@foolenough updated the pull request - view changes - changes since last import

[foolenough]

@yiwu-arbug I've added the test case which would fail without the fix patch.

I am currently benchmarking the blob db, and want to add a config option like Use_BlobDB_Value_Size. When the value size is large than the configure value, the value
is stored in blob file. And when the value size is small, it is store in LSM tree.
This may help improve performance in some cases. Is there any advice for this opinion?

What's your mean about NOT ready? How long would it take to use blob db in production?

[facebook-github-bot]

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[yiwu-arbug]

@foolenough Appreciate the fix. There a crashes we see in our shadow test need to fix, and I'm still investigating it. No ETA for production yet. Where do you plan to use blob db?

To me, option to store value in LSM is something blob db should have. How do you plan to implement it? We have a placeholder here which is unimplemented :/
https://github.com/facebook/rocksdb/blob/master/utilities/blob_db/blob_db.h#L55-L57