From 930cb0b9ee12c18eb461ef78748ed5b9bcf80d98 Mon Sep 17 00:00:00 2001
From: lovro <lovro@fb.com>
Date: Mon, 2 Dec 2013 14:59:23 -0800
Subject: [PATCH] Clarify CompactionFilter thread safety requirements

Summary: Documenting our discussion

Test Plan: make

Reviewers: dhruba, haobo

Reviewed By: dhruba

CC: igor

Differential Revision: https://reviews.facebook.net/D14403
---
 include/rocksdb/compaction_filter.h | 10 ++++++++++
 include/rocksdb/options.h           | 28 ++++++++++++++++++++--------
 util/options.cc                     |  6 +++---
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/include/rocksdb/compaction_filter.h b/include/rocksdb/compaction_filter.h
index cb55ac44f..f24132a6f 100644
--- a/include/rocksdb/compaction_filter.h
+++ b/include/rocksdb/compaction_filter.h
@@ -40,6 +40,16 @@ class CompactionFilter {
   // When the value is to be preserved, the application has the option
   // to modify the existing_value and pass it back through new_value.
   // value_changed needs to be set to true in this case.
+  //
+  // If multithreaded compaction is being used *and* a single CompactionFilter
+  // instance was supplied via Options::compaction_filter, this method may be
+  // called from different threads concurrently.  The application must ensure
+  // that the call is thread-safe.
+  //
+  // If the CompactionFilter was created by a factory, then it will only ever
+  // be used by a single thread that is doing the compaction run, and this
+  // call does not need to be thread-safe.  However, multiple filters may be
+  // in existence and operating concurrently.
   virtual bool Filter(int level,
                       const Slice& key,
                       const Slice& existing_value,
diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h
index 9a4644c8e..85c1db059 100644
--- a/include/rocksdb/options.h
+++ b/include/rocksdb/options.h
@@ -93,16 +93,33 @@ struct Options {
   // Default: nullptr
   shared_ptr<MergeOperator> merge_operator;
 
-  // The client must provide compaction_filter_factory if it requires a new
-  // compaction filter to be used for different compaction processes
+  // A single CompactionFilter instance to call into during compaction.
   // Allows an application to modify/delete a key-value during background
   // compaction.
-  // Ideally, client should specify only one of filter or factory.
+  //
+  // If the client requires a new compaction filter to be used for different
+  // compaction runs, it can specify compaction_filter_factory instead of this
+  // option.  The client should specify only one of the two.
   // compaction_filter takes precedence over compaction_filter_factory if
   // client specifies both.
+  //
+  // If multithreaded compaction is being used, the supplied CompactionFilter
+  // instance may be used from different threads concurrently and so should be
+  // thread-safe.
+  //
   // Default: nullptr
   const CompactionFilter* compaction_filter;
 
+  // This is a factory that provides compaction filter objects which allow
+  // an application to modify/delete a key-value during background compaction.
+  //
+  // A new filter will be created on each compaction run.  If multithreaded
+  // compaction is being used, each created CompactionFilter will only be used
+  // from a single thread and so does not need to be thread-safe.
+  //
+  // Default: a factory that doesn't provide any object
+  std::shared_ptr<CompactionFilterFactory> compaction_filter_factory;
+
   // If true, the database will be created if it is missing.
   // Default: false
   bool create_if_missing;
@@ -600,11 +617,6 @@ struct Options {
   // Table and TableBuilder.
   std::shared_ptr<TableFactory> table_factory;
 
-  // This is a factory that provides compaction filter objects which allow
-  // an application to modify/delete a key-value during background compaction.
-  // Default: a factory that doesn't provide any object
-  std::shared_ptr<CompactionFilterFactory> compaction_filter_factory;
-
   // This option allows user to to collect their own interested statistics of
   // the tables.
   // Default: emtpy vector -- no user-defined statistics collection will be
diff --git a/util/options.cc b/util/options.cc
index 7fa7586e3..fffcce0a1 100644
--- a/util/options.cc
+++ b/util/options.cc
@@ -25,6 +25,9 @@ Options::Options()
     : comparator(BytewiseComparator()),
       merge_operator(nullptr),
       compaction_filter(nullptr),
+      compaction_filter_factory(
+          std::shared_ptr<CompactionFilterFactory>(
+            new DefaultCompactionFilterFactory())),
       create_if_missing(false),
       error_if_exists(false),
       paranoid_checks(false),
@@ -97,9 +100,6 @@ Options::Options()
       memtable_factory(std::shared_ptr<SkipListFactory>(new SkipListFactory)),
       table_factory(
         std::shared_ptr<TableFactory>(new BlockBasedTableFactory())),
-      compaction_filter_factory(
-          std::shared_ptr<CompactionFilterFactory>(
-            new DefaultCompactionFilterFactory())),
       inplace_update_support(false),
       inplace_update_num_locks(10000) {
   assert(memtable_factory.get() != nullptr);