Disable snapshot refresh feature when snap_refresh_nanos is 0 (#5724)

Summary:
The comments of snap_refresh_nanos advertise that the snapshot refresh feature will be disabled when the option is set to 0. This contract is however not honored in the code: https://github.com/facebook/rocksdb/pull/5278
The patch fixes that and also adds an assert to ensure that the feature is not used when the option  is zero.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/5724

Differential Revision: D16918185

Pulled By: maysamyabandeh

fbshipit-source-id: fec167287df7d85093e087fc39c0eb243e3bbd7e

diff --git a/HISTORY.md b/HISTORY.md
index d83bde76c6fe853131db956b60259178c33b1dca..06c2bd0e34d7f73561be64e7aaa6c76b8400f777 100644 (file)
--- a/HISTORY.md
+++ b/HISTORY.md
@@ -1,4 +1,8 @@
 # Rocksdb Change Log
+## 6.2.4 (9/18/2019)
+### Bug Fixes
+* Disable snap_refresh_nanos by default. The feature is to be deprecated in the next release.
+
 ## 6.2.3 (9/3/2019)
 ### Bug Fixes
 * Fix a bug in file ingestion caused by incorrect file number allocation when the number of column families involved in the ingestion exceeds 2.

diff --git a/db/compaction_iterator.h b/db/compaction_iterator.h
index 6ab43b1becfcc317b34c16c9a0092e3a78699062..df25eaa9b52ad1019b20aebd5d98bbb980a36766 100644 (file)
--- a/db/compaction_iterator.h
+++ b/db/compaction_iterator.h
@@ -39,6 +39,7 @@ class SnapshotListFetchCallback {
   virtual void Refresh(std::vector<SequenceNumber>* snapshots,
                        SequenceNumber max) = 0;
   inline bool TimeToRefresh(const size_t key_index) {
+    assert(snap_refresh_nanos_ != 0);
     // skip the key if key_index % every_nth_key (which is of power 2) is not 0.
     if ((key_index & every_nth_key_minus_one_) != 0) {
       return false;

diff --git a/db/compaction_job_test.cc b/db/compaction_job_test.cc
index 60394cc9735a038f39e1939630809bb97d8e2158..49f7384fee5a3cf067bc5bcfb289056aba3bd330 100644 (file)
--- a/db/compaction_job_test.cc
+++ b/db/compaction_job_test.cc
@@ -966,7 +966,7 @@ TEST_F(CompactionJobTest, SnapshotRefresh) {
    public:
     SnapshotListFetchCallbackTest(Env* env, Random64& rand,
                                   std::vector<SequenceNumber>* snapshots)
-        : SnapshotListFetchCallback(env, 0 /*no time delay*/,
+        : SnapshotListFetchCallback(env, 1 /*short time delay*/,
                                     1 /*fetch after each key*/),
           rand_(rand),
           snapshots_(snapshots) {}

diff --git a/db/db_impl_compaction_flush.cc b/db/db_impl_compaction_flush.cc
index 3fbf24e49f814dce89104918d36d9318b9cc0051..865acf5c5599360f8fa2057df95d22a7b41ebe57 100644 (file)
--- a/db/db_impl_compaction_flush.cc
+++ b/db/db_impl_compaction_flush.cc
@@ -1007,8 +1007,10 @@ Status DBImpl::CompactFilesImpl(
       c->mutable_cf_options()->paranoid_file_checks,
       c->mutable_cf_options()->report_bg_io_stats, dbname_,
       &compaction_job_stats, Env::Priority::USER,
-      immutable_db_options_.max_subcompactions <= 1 ? &fetch_callback
-                                                    : nullptr);
+      immutable_db_options_.max_subcompactions <= 1 &&
+              c->mutable_cf_options()->snap_refresh_nanos > 0
+          ? &fetch_callback
+          : nullptr);
 
   // Creating a compaction influences the compaction score because the score
   // takes running compactions into account (by skipping files that are already
@@ -2669,8 +2671,10 @@ Status DBImpl::BackgroundCompaction(bool* made_progress,
         &event_logger_, c->mutable_cf_options()->paranoid_file_checks,
         c->mutable_cf_options()->report_bg_io_stats, dbname_,
         &compaction_job_stats, thread_pri,
-        immutable_db_options_.max_subcompactions <= 1 ? &fetch_callback
-                                                      : nullptr);
+        immutable_db_options_.max_subcompactions <= 1 &&
+                c->mutable_cf_options()->snap_refresh_nanos > 0
+            ? &fetch_callback
+            : nullptr);
     compaction_job.Prepare();
 
     NotifyOnCompactionBegin(c->column_family_data(), c.get(), status,