From 4ff493bb38e951a59150601cd2ee3ff225df2862 Mon Sep 17 00:00:00 2001 From: Maysam Yabandeh Date: Tue, 20 Aug 2019 11:38:15 -0700 Subject: [PATCH] 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 --- HISTORY.md | 4 ++++ db/compaction/compaction_iterator.h | 1 + db/compaction/compaction_job_test.cc | 2 +- db/db_impl/db_impl_compaction_flush.cc | 12 ++++++++---- include/rocksdb/version.h | 2 +- 5 files changed, 15 insertions(+), 6 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index d8ab8a98b..d64b9e875 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,4 +1,8 @@ # Rocksdb Change Log +## 6.3.3 (8/20/2019) +### Bug Fixes +* Fix a bug where the compaction snapshot refresh feature is not disabled as advertised when `snap_refresh_nanos` is set to 0.. + ## 6.3.2 (8/15/2019) ### Public API Change * The semantics of the per-block-type block read counts in the performance context now match those of the generic block_read_count. diff --git a/db/compaction/compaction_iterator.h b/db/compaction/compaction_iterator.h index 86a2b87b2..2bf847e2e 100644 --- a/db/compaction/compaction_iterator.h +++ b/db/compaction/compaction_iterator.h @@ -39,6 +39,7 @@ class SnapshotListFetchCallback { virtual void Refresh(std::vector* 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/compaction_job_test.cc b/db/compaction/compaction_job_test.cc index add491189..b813966bc 100644 --- a/db/compaction/compaction_job_test.cc +++ b/db/compaction/compaction_job_test.cc @@ -964,7 +964,7 @@ TEST_F(CompactionJobTest, SnapshotRefresh) { public: SnapshotListFetchCallbackTest(Env* env, Random64& rand, std::vector* 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/db_impl_compaction_flush.cc b/db/db_impl/db_impl_compaction_flush.cc index 4c230ea38..7c538a792 100644 --- a/db/db_impl/db_impl_compaction_flush.cc +++ b/db/db_impl/db_impl_compaction_flush.cc @@ -1008,8 +1008,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 @@ -2711,8 +2713,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, diff --git a/include/rocksdb/version.h b/include/rocksdb/version.h index 6a5c27e75..27fcac0c6 100644 --- a/include/rocksdb/version.h +++ b/include/rocksdb/version.h @@ -6,7 +6,7 @@ #define ROCKSDB_MAJOR 6 #define ROCKSDB_MINOR 3 -#define ROCKSDB_PATCH 2 +#define ROCKSDB_PATCH 3 // Do not use these. We made the mistake of declaring macros starting with // double underscore. Now we have to live with our choice. We'll deprecate these