From ffe1e4b8207650f50ba2202020b7ad2fad891b11 Mon Sep 17 00:00:00 2001
From: Peter Dillinger <peterd@fb.com>
Date: Wed, 19 Jan 2022 18:10:36 -0800
Subject: [PATCH] Make some FilterPolicy deprecations more clear (#9403)

Summary:
The old block-based filter has been deprecated for years, but
this makes that more clear by marking the functions specific to it and
logging a warning when the feature is used.

It is deprecated because of performance. In that old design, you have to
binary search through the full SST index before a bloom filter query, which
is much more expensive than a bloom query itself.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/9403

Test Plan:
Used db_bench with and without -use_block_based_filter,
running at the same time

    TEST_TMPDIR=/dev/shm/rocksdb ./db_bench -benchmarks=fillrandom,readrandom -num=10000000 -duration=20 -disable_wal=1 -write_buffer_size=10000000 -bloom_bits=16 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=10000 -fifo_compaction_allow_compaction=0

No significant difference in construction time but 3x slower readrandom
with -use_block_based_filter:
readrandom   :     100.517 micros/op 9948 ops/sec;    1.1 MB/s
vs.
readrandom   :      33.368 micros/op 29968 ops/sec;    3.3 MB/s

Also saw deprecation message (just once) in LOG only with
-use_block_based_filter

Reviewed By: ajkr

Differential Revision: D33673202

Pulled By: pdillinger

fbshipit-source-id: 99f6f0eff619408d9e5f7ef546954ed0be6c7a5b
---
 include/rocksdb/filter_policy.h    | 9 ++++++++-
 table/block_based/filter_policy.cc | 7 +++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/rocksdb/filter_policy.h b/include/rocksdb/filter_policy.h
index 58d740e35..01a29f27a 100644
--- a/include/rocksdb/filter_policy.h
+++ b/include/rocksdb/filter_policy.h
@@ -177,6 +177,9 @@ class FilterPolicy {
   // passed to methods of this type.
   virtual const char* Name() const = 0;
 
+  // DEPRECATED: This function is part of the deprecated block-based
+  // filter, which will be removed in a future release.
+  //
   // keys[0,n-1] contains a list of keys (potentially with duplicates)
   // that are ordered according to the user supplied comparator.
   // Append a filter that summarizes keys[0,n-1] to *dst.
@@ -186,6 +189,9 @@ class FilterPolicy {
   virtual void CreateFilter(const Slice* keys, int n,
                             std::string* dst) const = 0;
 
+  // DEPRECATED: This function is part of the deprecated block-based
+  // filter, which will be removed in a future release.
+  //
   // "filter" contains the data appended by a preceding call to
   // CreateFilter() on this class.  This method must return true if
   // the key was in the list of keys passed to CreateFilter().
@@ -198,6 +204,7 @@ class FilterPolicy {
   // NOTE: This function is only called by GetBuilderWithContext() below for
   // custom FilterPolicy implementations. Thus, it is not necessary to
   // override this function if overriding GetBuilderWithContext().
+  // DEPRECATED: This function will be removed in a future release.
   virtual FilterBitsBuilder* GetFilterBitsBuilder() const { return nullptr; }
 
   // A newer variant of GetFilterBitsBuilder that allows a FilterPolicy
@@ -282,7 +289,7 @@ extern const FilterPolicy* NewBloomFilterPolicy(
 extern const FilterPolicy* NewRibbonFilterPolicy(
     double bloom_equivalent_bits_per_key, int bloom_before_level = 0);
 
-// Old name and old default behavior
+// Old name and old default behavior (DEPRECATED)
 inline const FilterPolicy* NewExperimentalRibbonFilterPolicy(
     double bloom_equivalent_bits_per_key) {
   return NewRibbonFilterPolicy(bloom_equivalent_bits_per_key, -1);
diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc
index ae5da2d7b..4e49fbbdd 100644
--- a/table/block_based/filter_policy.cc
+++ b/table/block_based/filter_policy.cc
@@ -1257,6 +1257,13 @@ FilterBitsBuilder* BloomFilterPolicy::GetBuilderWithContext(
         }
         break;
       case kDeprecatedBlock:
+        if (context.info_log && !warned_.load(std::memory_order_relaxed)) {
+          warned_ = true;
+          ROCKS_LOG_WARN(context.info_log,
+                         "Using deprecated block-based Bloom filter is "
+                         "inefficient (%d bits per key).",
+                         whole_bits_per_key_);
+        }
         return nullptr;
       case kFastLocalBloom:
         return new FastLocalBloomBitsBuilder(