Compaction filter support for (new) BlobDB #7974

riversand963 · 2021-02-18T07:49:27Z

Allow applications to implement a custom compaction filter and pass it to BlobDB.

The compaction filter's custom logic can operate on blobs.
To do so, application needs to subclass CompactionFilter abstract class and implement FilterV2() method.
Optionally, a method called ShouldFilterBlobByKey() can be implemented if application's custom logic rely solely
on the key to make a decision without reading the blob, thus saving extra IO. Examples can be found in
db/blob/db_blob_compaction_test.cc.

Test plan:
make check

facebook-github-bot

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

ltamasi

Thanks a lot for the PR @riversand963 ! Could you also update db_bench and db_stress / db_crashtest.py so they allow using compaction filters with the new BlobDB?

HISTORY.md

include/rocksdb/compaction_filter.h

db/column_family.cc

db/compaction/compaction_iterator.cc

ltamasi · 2021-02-22T20:07:28Z

db/compaction/compaction_iterator.cc

-    }
+  if (CompactionFilter::Decision::kUndetermined == filter) {
+    filter = CompactionFilter::Decision::kKeep;
+  } else if (CompactionFilter::Decision::kChangeBlobIndex == filter &&


👍

The same holds for kIOError as well (i.e. only the stacked BlobDB's internal filter could potentially return it).

I think that's added already.

Sorry, I might be missing something; where is it checked? I can see an assertion further down but not a check like this...

Oh I see you are referring to returning an error. Just added.

Again, maybe I'm misreading the code but now I think we have two checks for CompactionFilter::Decision::kChangeBlobIndex == filter && !compaction_filter_->IsStackedBlobDbInternalCompactionFilter() ? If so, we could remove this one (line 282-283)

db/compaction/compaction_iterator.cc

db/blob/db_blob_compaction_test.cc

riversand963 · 2021-02-22T21:41:20Z

Thanks a lot for the PR @riversand963 ! Could you also update db_bench and db_stress / db_crashtest.py so they allow using compaction filters with the new BlobDB?

Thanks @ltamasi for the review.

Can this be in a separate PR?

ltamasi · 2021-02-22T21:43:53Z

Thanks a lot for the PR @riversand963 ! Could you also update db_bench and db_stress / db_crashtest.py so they allow using compaction filters with the new BlobDB?

Thanks @ltamasi for the review.

Can this be in a separate PR?

Sure, absolutely! Thanks!

facebook-github-bot · 2021-02-23T23:06:27Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-02-23T23:27:56Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

riversand963 · 2021-02-23T23:31:21Z

Thanks @ltamasi for the detailed review!

facebook-github-bot · 2021-02-23T23:33:43Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

facebook-github-bot · 2021-02-24T00:36:50Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

db/compaction/compaction_iterator.cc

facebook-github-bot · 2021-02-24T22:32:17Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

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

ltamasi

Thanks once again for the change @riversand963 !

ltamasi · 2021-02-25T18:46:11Z

db/blob/db_blob_compaction_test.cc

+
+namespace ROCKSDB_NAMESPACE {
+
+class DBBlobCompactionTest : public DBTestBase {


Thanks for the really comprehensive unit tests! This is awesome :)

ltamasi · 2021-02-25T18:46:39Z

db/blob/db_blob_compaction_test.cc

+class DummyStackedBlobDbCompactionFilter : public CompactionFilter {
+ public:
+  explicit DummyStackedBlobDbCompactionFilter() = default;
+  const char* Name() const override {
+    return "rocksdb.compaction.filter.dummy.stacked.blobdb";
+  }
+  bool IsStackedBlobDbInternalCompactionFilter() const override { return true; }
+};


I think this is no longer in use

ltamasi · 2021-02-25T18:49:36Z

db/compaction/compaction_iterator.cc

-    }
+  if (CompactionFilter::Decision::kUndetermined == filter) {
+    filter = CompactionFilter::Decision::kKeep;
+  } else if (CompactionFilter::Decision::kChangeBlobIndex == filter &&


Again, maybe I'm misreading the code but now I think we have two checks for CompactionFilter::Decision::kChangeBlobIndex == filter && !compaction_filter_->IsStackedBlobDbInternalCompactionFilter() ? If so, we could remove this one (line 282-283)

facebook-github-bot · 2021-02-25T19:49:01Z

@riversand963 has updated the pull request. You must reimport the pull request before landing.

riversand963 · 2021-02-25T19:49:32Z

Thanks @ltamasi for the review!

facebook-github-bot

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

facebook-github-bot · 2021-02-26T00:32:51Z

@riversand963 merged this pull request in cef4a6c.

Summary: Allow applications to implement a custom compaction filter and pass it to BlobDB. The compaction filter's custom logic can operate on blobs. To do so, application needs to subclass `CompactionFilter` abstract class and implement `FilterV2()` method. Optionally, a method called `ShouldFilterBlobByKey()` can be implemented if application's custom logic rely solely on the key to make a decision without reading the blob, thus saving extra IO. Examples can be found in db/blob/db_blob_compaction_test.cc. Pull Request resolved: facebook#7974 Test Plan: make check Reviewed By: ltamasi Differential Revision: D26509280 Pulled By: riversand963 fbshipit-source-id: 59f9ae5614c4359de32f4f2b16684193cc537b39

riversand963 requested a review from ltamasi February 18, 2021 07:49

facebook-github-bot added the CLA Signed label Feb 18, 2021

facebook-github-bot reviewed Feb 18, 2021

View reviewed changes

ltamasi reviewed Feb 22, 2021

View reviewed changes

riversand963 force-pushed the blob-compaction-filter branch from 39e5991 to 0bb355e Compare February 23, 2021 23:27

facebook-github-bot reviewed Feb 23, 2021

View reviewed changes

riversand963 requested a review from ltamasi February 23, 2021 23:29

facebook-github-bot reviewed Feb 23, 2021

View reviewed changes

facebook-github-bot reviewed Feb 24, 2021

View reviewed changes

ltamasi reviewed Feb 24, 2021

View reviewed changes

db/compaction/compaction_iterator.cc Outdated Show resolved Hide resolved

facebook-github-bot reviewed Feb 24, 2021

View reviewed changes

ltamasi approved these changes Feb 25, 2021

View reviewed changes

riversand963 added 10 commits February 25, 2021 11:47

Update HISTORY

0c91c95

Return early from InvokeFilterIfNeeded()

76a565a

Add a new method

591c9d7

Add a new test for BlobDB compaction

d7f40d5

Sanity check on options

b5d5bce

Address review comments

27a29a3

Update HISTORY

3281d9f

Minor

206999b

Address review comments

a0cc3df

Add more test coverage

785204a

riversand963 added 2 commits February 25, 2021 11:47

Address review comment

8a13ecd

Address review comments

1a2d299

riversand963 force-pushed the blob-compaction-filter branch from 761f270 to 1a2d299 Compare February 25, 2021 19:48

facebook-github-bot reviewed Feb 25, 2021

View reviewed changes

facebook-github-bot closed this in cef4a6c Feb 26, 2021

facebook-github-bot added the Merged label Feb 26, 2021

riversand963 deleted the blob-compaction-filter branch February 26, 2021 01:01

tabokie mentioned this pull request May 20, 2022

work with 6.29.tikv tikv/titan#246

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compaction filter support for (new) BlobDB #7974

Compaction filter support for (new) BlobDB #7974

riversand963 commented Feb 18, 2021

facebook-github-bot left a comment

ltamasi left a comment

ltamasi Feb 22, 2021

riversand963 Feb 23, 2021

ltamasi Feb 24, 2021

riversand963 Feb 24, 2021

ltamasi Feb 25, 2021

riversand963 commented Feb 22, 2021

ltamasi commented Feb 22, 2021 •

edited

Loading

facebook-github-bot commented Feb 23, 2021

facebook-github-bot commented Feb 23, 2021

facebook-github-bot left a comment

riversand963 commented Feb 23, 2021

facebook-github-bot commented Feb 23, 2021

facebook-github-bot left a comment

facebook-github-bot commented Feb 24, 2021

facebook-github-bot left a comment

facebook-github-bot commented Feb 24, 2021

facebook-github-bot left a comment

ltamasi left a comment

ltamasi Feb 25, 2021

ltamasi Feb 25, 2021

ltamasi Feb 25, 2021

facebook-github-bot commented Feb 25, 2021

riversand963 commented Feb 25, 2021

facebook-github-bot left a comment

facebook-github-bot commented Feb 26, 2021


		namespace ROCKSDB_NAMESPACE {

		class DBBlobCompactionTest : public DBTestBase {

Compaction filter support for (new) BlobDB #7974

Compaction filter support for (new) BlobDB #7974

Conversation

riversand963 commented Feb 18, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

ltamasi left a 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

riversand963 commented Feb 22, 2021

ltamasi commented Feb 22, 2021 • edited Loading

facebook-github-bot commented Feb 23, 2021

facebook-github-bot commented Feb 23, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

riversand963 commented Feb 23, 2021

facebook-github-bot commented Feb 23, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 24, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 24, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

ltamasi left a 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

facebook-github-bot commented Feb 25, 2021

riversand963 commented Feb 25, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 26, 2021

ltamasi commented Feb 22, 2021 •

edited

Loading