Add the support of SCALING for bloom filter #1721

zncleon · 2023-08-31T18:06:40Z

This PR supports the "SCALING" of bloom filter. When the bloom filter is full, it will add a new bloom filter into bloom chain if is "SCALING"，otherwise it will return an error.

torwig · 2023-08-31T18:14:31Z

src/storage/redis_metadata.h

@@ -245,4 +245,9 @@ class BloomChainMetadata : public Metadata {
  ///
  /// @return the total capacity value
  uint32_t GetCapacity() const;
+
+  /// Check the bloom chain is scaling or not


I think it's needless to comment on simple methods such as IsScaling or GetCapacity - they have self-explanatory names. BTW, the implementation of IsScaling is so short/simple that you can write it in the header file.

PragmaTwice · 2023-09-01T02:04:41Z

src/types/redis_bloom_chain.cc

-      return rocksdb::Status::Aborted("filter is full");
+    if (metadata.size + 1 > metadata.GetCapacity()) {
+      if (metadata.IsScaling()) {
+        s = createBloomFilter(ns_key, &metadata);


the status seems not checked

I'm not sure it can be done like this. Now we have holding a "lock" here. And createBloomFilter will directly call Write finally

I propose a createBloomFilterInBatch or other, the interface would just put the new bloomFilter in batch. The previous create can make use of this, and here we can just put it in batch first?

src/types/redis_bloom_chain.cc

mapleFU · 2023-09-04T13:39:45Z

src/types/redis_bloom_chain.cc

+
+rocksdb::Status BloomChain::createBloomFilter(const Slice &ns_key, BloomChainMetadata *metadata) {
+  uint32_t bloom_filter_bytes = BlockSplitBloomFilter::OptimalNumOfBytes(
+      static_cast<uint32_t>(metadata->base_capacity * pow(metadata->expansion, metadata->n_filters)),


Might not strong related to this patch, should we enforce a "hardlimit" to avoid too large BloomFilter?

cc @PragmaTwice @git-hulk

Does Redis have some limits here?

RedisBloom has a maximum Cuckoo Filter n_filters. And don't have limit

I think our bitmap has some limits. And BlockSplitBloomFilter limit 128MB, perhaps if filter is too large, we need to reject and return to user?

128MiB is also too large for one key, maybe we can improve this by separating them into multi subkeys in future.

Previously I talked with @zncleon . Split into subkeys doesn't work like bitmap, since bloom filter would tent to random access all bits in it's space. Maybe a maximum size would help?

zncleon · 2023-09-09T06:16:07Z

If error and return after CreateBloomfilter, it will left zombie bloomfilter. So I Update the function interfaces about CreateBloomfilterInBatch and BloomAdd. Using only one write will also get high performance.

cc @mapleFU

src/storage/redis_metadata.h

mapleFU

Rest LGTM, thanks!

mapleFU · 2023-09-11T13:37:48Z

src/types/redis_bloom_chain.cc

-  auto batch = storage_->GetWriteBatchBase();
-  batch->Put(bf_key, block_split_bloom_filter.GetData());
-  return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
+  *bf_data = block_split_bloom_filter.GetData();


This interface is ok, but what about adding a MoveData to avoid an around of copying?

From a glance, I think we just add a string&& GetData() &&.

src/types/redis_bloom_chain.cc

mapleFU · 2023-09-11T13:42:16Z

Oh seems Twice has refactor the storage, so you mind need to solve the conflicts.

Co-authored-by: mwish <[email protected]>

PragmaTwice · 2023-09-11T14:21:00Z

src/types/redis_bloom_chain.cc

-  auto batch = storage_->GetWriteBatchBase();
-  batch->Put(bf_key, block_split_bloom_filter.GetData());
-  return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
+  *bf_data = block_split_bloom_filter.GetData();


Suggested change

*bf_data = block_split_bloom_filter.GetData();

*bf_data = std::move(block_split_bloom_filter).GetData();

And add a overload to GetData:

std::string&& GetData() && { return data_; }

cc @mapleFU that seems better than a MoveData method.

Ok, this also LGTM

mapleFU

LGTM

mapleFU · 2023-09-11T15:21:24Z

src/types/redis_bloom_chain.h

-  rocksdb::Status bloomAdd(const Slice &bf_key, const std::string &item);
+  void createBloomFilterInBatch(const Slice &ns_key, BloomChainMetadata *metadata,
+                                ObserverOrUniquePtr<rocksdb::WriteBatchBase> &batch, std::string *bf_data);
+  static void bloomAdd(const std::string &item, std::string *bf_data);


Nit: when using pointer as an argument, it looks like an output. Maybe we can add a comment to mark this like:

/// bf_data: [in/out] The content string of bloomfilter.

src/types/bloom_filter.h

mapleFU · 2023-09-11T17:09:08Z

src/types/bloom_filter.h

-  const std::string& GetData() { return data_; }
+  const std::string& GetData() const& { return data_; }
+
+  std::string GetData() const&& { return data_; }


Emmm would this need a const? You can take a look at how StatusOr does this:

kvrocks/src/common/status.h

Line 276 in 2ecaef2

ValueType&& GetValue() && {

PragmaTwice · 2023-09-12T02:13:16Z

src/types/bloom_filter.h

-  const std::string& GetData() { return data_; }
+  const std::string& GetData() const& { return data_; }
+
+  std::string GetData() const&& { return data_; }


Suggested change

std::string GetData() const&& { return data_; }

std::string&& GetData() && { return std::move(data_); }

PragmaTwice · 2023-09-12T05:57:28Z

Thanks for your contribution!

Add the support of scaling bloom filter

d16abea

torwig reviewed Aug 31, 2023

View reviewed changes

PragmaTwice changed the title ~~Add the support of scalable bloom filter~~ Add the support of SCALING for bloom filter Sep 1, 2023

Merge branch 'unstable' into dev_scaling_bloom

94bc62c

PragmaTwice reviewed Sep 1, 2023

View reviewed changes

mapleFU mentioned this pull request Sep 1, 2023

[OSPP] Tracking issues: RedisBloom support #1696

Closed

11 tasks

Fix code

616cb5b

mapleFU reviewed Sep 1, 2023

View reviewed changes

src/types/redis_bloom_chain.cc Outdated Show resolved Hide resolved

zncleon and others added 2 commits September 1, 2023 16:25

Fix scaling bfkey list bugs

8b54c89

Merge branch 'unstable' into dev_scaling_bloom

afbbfb3

PragmaTwice requested a review from mapleFU September 3, 2023 02:48

Merge branch 'unstable' into dev_scaling_bloom

4210137

mapleFU reviewed Sep 4, 2023

View reviewed changes

PragmaTwice requested a review from mapleFU September 7, 2023 14:59

zncleon force-pushed the dev_scaling_bloom branch from a4d7274 to 4210137 Compare September 9, 2023 06:11

Update some functions interface

395e5aa

Merge branch 'unstable' into dev_scaling_bloom

527ac7b

mapleFU reviewed Sep 11, 2023

View reviewed changes

src/storage/redis_metadata.h Show resolved Hide resolved

mapleFU reviewed Sep 11, 2023

View reviewed changes

zncleon and others added 2 commits September 11, 2023 22:04

Update src/types/redis_bloom_chain.cc

d9b1b59

Co-authored-by: mwish <[email protected]>

Merge branch 'unstable' into dev_scaling_bloom

4f4975e

PragmaTwice reviewed Sep 11, 2023

View reviewed changes

Update GetData interface

82bd9e5

mapleFU previously approved these changes Sep 11, 2023

View reviewed changes

mapleFU requested review from git-hulk, torwig and PragmaTwice September 11, 2023 15:22

PragmaTwice reviewed Sep 11, 2023

View reviewed changes

src/types/bloom_filter.h Outdated Show resolved Hide resolved

Update GetData interface

320502b

zncleon dismissed mapleFU’s stale review via 320502b September 11, 2023 16:16

mapleFU reviewed Sep 11, 2023

View reviewed changes

PragmaTwice reviewed Sep 12, 2023

View reviewed changes

Update GetData interface

0d741b3

mapleFU approved these changes Sep 12, 2023

View reviewed changes

Merge branch 'unstable' into dev_scaling_bloom

07dbeda

PragmaTwice merged commit 11a0140 into apache:unstable Sep 12, 2023
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the support of SCALING for bloom filter #1721

Add the support of SCALING for bloom filter #1721

zncleon commented Aug 31, 2023

torwig Aug 31, 2023

PragmaTwice Sep 1, 2023 •

edited

Loading

mapleFU Sep 1, 2023

mapleFU Sep 4, 2023

PragmaTwice Sep 4, 2023

mapleFU Sep 4, 2023

git-hulk Sep 4, 2023 •

edited

Loading

mapleFU Sep 4, 2023

zncleon commented Sep 9, 2023 •

edited

Loading

mapleFU left a comment

mapleFU Sep 11, 2023

PragmaTwice Sep 11, 2023 •

edited

Loading

mapleFU commented Sep 11, 2023

PragmaTwice Sep 11, 2023 •

edited

Loading

PragmaTwice Sep 11, 2023 •

edited

Loading

mapleFU Sep 11, 2023

mapleFU left a comment

mapleFU Sep 11, 2023

mapleFU Sep 11, 2023 •

edited

Loading

PragmaTwice Sep 12, 2023

PragmaTwice commented Sep 12, 2023

	*bf_data = block_split_bloom_filter.GetData();
	*bf_data = std::move(block_split_bloom_filter).GetData();

	std::string GetData() const&& { return data_; }
	std::string&& GetData() && { return std::move(data_); }

Add the support of SCALING for bloom filter #1721

Add the support of SCALING for bloom filter #1721

Conversation

zncleon commented Aug 31, 2023

Choose a reason for hiding this comment

PragmaTwice Sep 1, 2023 • 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

git-hulk Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zncleon commented Sep 9, 2023 • edited Loading

mapleFU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

mapleFU commented Sep 11, 2023

PragmaTwice Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

PragmaTwice Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice commented Sep 12, 2023

PragmaTwice Sep 1, 2023 •

edited

Loading

git-hulk Sep 4, 2023 •

edited

Loading

zncleon commented Sep 9, 2023 •

edited

Loading

PragmaTwice Sep 11, 2023 •

edited

Loading

PragmaTwice Sep 11, 2023 •

edited

Loading

PragmaTwice Sep 11, 2023 •

edited

Loading

mapleFU Sep 11, 2023 •

edited

Loading