Deprecate CompactionFilter::IgnoreSnapshots() = false (#4954)

Summary:
We found that the behavior of CompactionFilter::IgnoreSnapshots() = false isn't
what we have expected. We thought that snapshot will always be preserved.
However, we just realized that, if no snapshot is created while compaction
starts, and a snapshot is created after that, the data seen from the snapshot
can successfully be dropped by the compaction. This creates a strange behavior
to the feature, which is hard to explain. Like what is documented in code
comment, this feature is not very useful with snapshot anyway. The decision
is to deprecate the feature.

We keep the function to avoid to break users code. However, we will fail
compactions if false is returned.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/4954

Differential Revision: D13981900

Pulled By: siying

fbshipit-source-id: 2db8c2c3865acd86a28dca625945d1481b1d1e36

2 files changed, 15 insertions, 21 deletions

diff --git a/include/rocksdb/compaction_filter.h b/include/rocksdb/compaction_filter.h
index 98f86c281..f40e39da6 100644
--- a/include/rocksdb/compaction_filter.h
+++ b/include/rocksdb/compaction_filter.h
@@ -75,14 +75,11 @@ class CompactionFilter {
   // to modify the existing_value and pass it back through new_value.
   // value_changed needs to be set to true in this case.
   //
-  // If you use snapshot feature of RocksDB (i.e. call GetSnapshot() API on a
-  // DB* object), CompactionFilter might not be very useful for you. Due to
-  // guarantees we need to maintain, compaction process will not call Filter()
-  // on any keys that were written before the latest snapshot. In other words,
-  // compaction will only call Filter() on keys written after your most recent
-  // call to GetSnapshot(). In most cases, Filter() will not be called very
-  // often. This is something we're fixing. See the discussion at:
-  // https://www.facebook.com/groups/mysqlonrocksdb/permalink/999723240091865/
+  // Note that RocksDB snapshots (i.e. call GetSnapshot() API on a
+  // DB* object) will not guarantee to preserve the state of the DB with
+  // CompactionFilter. Data seen from a snapshot might disppear after a
+  // compaction finishes. If you use snapshots, think twice about whether you
+  // want to use compaction filter and whether you are using it in a safe way.
   //
   // If multithreaded compaction is being used *and* a single CompactionFilter
   // instance was supplied via Options::compaction_filter, this method may be
@@ -135,9 +132,9 @@ class CompactionFilter {
   //
   //      Caveats:
   //       - The keys are skipped even if there are snapshots containing them,
-  //         as if IgnoreSnapshots() was true; i.e. values removed
-  //         by kRemoveAndSkipUntil can disappear from a snapshot - beware
-  //         if you're using TransactionDB or DB::GetSnapshot().
+  //         i.e. values removed by kRemoveAndSkipUntil can disappear from a
+  //         snapshot - beware if you're using TransactionDB or
+  //         DB::GetSnapshot().
   //       - If value for a key was overwritten or merged into (multiple Put()s
   //         or Merge()s), and compaction filter skips this key with
   //         kRemoveAndSkipUntil, it's possible that it will remove only
@@ -176,15 +173,12 @@ class CompactionFilter {
     return Decision::kKeep;
   }
 
-  // By default, compaction will only call Filter() on keys written after the
-  // most recent call to GetSnapshot(). However, if the compaction filter
-  // overrides IgnoreSnapshots to make it return true, the compaction filter
-  // will be called even if the keys were written before the last snapshot.
-  // This behavior is to be used only when we want to delete a set of keys
-  // irrespective of snapshots. In particular, care should be taken
-  // to understand that the values of these keys will change even if we are
-  // using a snapshot.
-  virtual bool IgnoreSnapshots() const { return false; }
+  // This function is deprecated. Snapshots will always be ignored for
+  // compaction filters, because we realized that not ignoring snapshots doesn't
+  // provide the gurantee we initially thought it would provide. Repeatable
+  // reads will not be guaranteed anyway. If you override the function and
+  // returns false, we will fail the compaction.
+  virtual bool IgnoreSnapshots() const { return true; }
 
   // Returns a name that identifies this compaction filter.
   // The name will be printed to LOG file on start up for diagnosis.
diff --git a/include/rocksdb/utilities/lua/rocks_lua_compaction_filter.h b/include/rocksdb/utilities/lua/rocks_lua_compaction_filter.h
index 71dd7ee5b..c0154aae2 100644
--- a/include/rocksdb/utilities/lua/rocks_lua_compaction_filter.h
+++ b/include/rocksdb/utilities/lua/rocks_lua_compaction_filter.h
@@ -99,7 +99,7 @@ struct RocksLuaCompactionFilterOptions {
   bool ignore_value = false;
 
   // A boolean flag to determine whether to ignore snapshots.
-  bool ignore_snapshots = false;
+  bool ignore_snapshots = true;
 
   // When specified a non-null pointer, the first "error_limit_per_filter"
   // errors of each CompactionFilter that is lua related will be included