From c7ccbb33a641db418fd3b53ff63c5952e435db15 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Thu, 28 Jul 2022 17:07:36 -0700 Subject: [PATCH] Allow manual compactions to run in parallel by default (#10317) Summary: This PR changes the default value of `CompactRangeOptions::exclusive_manual_compaction` from true to false so manual `CompactRange()`s can run in parallel with other compactions. I believe no artificial parallelism restriction is the intuitive behavior so feel the old default value is a trap, which I have fallen into several times, including yesterday. `CompactRangeOptions::exclusive_manual_compaction == false` has been used in both our correctness test and in production for years so should be reasonably safe. Pull Request resolved: https://github.com/facebook/rocksdb/pull/10317 Reviewed By: jay-zhuang Differential Revision: D37659392 Pulled By: ajkr fbshipit-source-id: 504915e978bbe300b79483d064070c75e93d91e5 --- HISTORY.md | 1 + db/db_compaction_test.cc | 5 +++-- db/db_impl/db_impl_compaction_flush.cc | 3 +-- include/rocksdb/options.h | 7 +++++-- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 0a976033e..db9ae89ce 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -10,6 +10,7 @@ ### Public API changes * Removed Customizable support for RateLimiter and removed its CreateFromString() and Type() functions. +* `CompactRangeOptions::exclusive_manual_compaction` is now false by default. This ensures RocksDB does not introduce artificial parallelism limitations by default. ### Bug Fixes * Fix a bug where `GenericRateLimiter` could revert the bandwidth set dynamically using `SetBytesPerSecond()` when a user configures a structure enclosing it, e.g., using `GetOptionsFromString()` to configure an `Options` that references an existing `RateLimiter` object. diff --git a/db/db_compaction_test.cc b/db/db_compaction_test.cc index 57155f441..d4b96c757 100644 --- a/db/db_compaction_test.cc +++ b/db/db_compaction_test.cc @@ -2660,8 +2660,9 @@ TEST_F(DBCompactionTest, ManualAutoRace) { TEST_SYNC_POINT("DBCompactionTest::ManualAutoRace:1"); // The auto compaction will wait until the manual compaction is registerd // before processing so that it will be cancelled. - ASSERT_OK(dbfull()->CompactRange(CompactRangeOptions(), handles_[1], nullptr, - nullptr)); + CompactRangeOptions cro; + cro.exclusive_manual_compaction = true; + ASSERT_OK(dbfull()->CompactRange(cro, handles_[1], nullptr, nullptr)); ASSERT_EQ("0,1", FilesPerLevel(1)); // Eventually the cancelled compaction will be rescheduled and executed. diff --git a/db/db_impl/db_impl_compaction_flush.cc b/db/db_impl/db_impl_compaction_flush.cc index 23abb5a34..7e517947b 100644 --- a/db/db_impl/db_impl_compaction_flush.cc +++ b/db/db_impl/db_impl_compaction_flush.cc @@ -1853,8 +1853,7 @@ Status DBImpl::RunManualCompaction( // jobs drops to zero. This used to be needed to ensure that this manual // compaction can compact any range of keys/files. Now it is optional // (see `CompactRangeOptions::exclusive_manual_compaction`). The use case for - // `exclusive_manual_compaction=true` (the default) is unclear beyond not - // trusting the new code. + // `exclusive_manual_compaction=true` is unclear beyond not trusting the code. // // HasPendingManualCompaction() is true when at least one thread is inside // RunManualCompaction(), i.e. during that time no other compaction will diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h index 7a58c1c09..917379e05 100644 --- a/include/rocksdb/options.h +++ b/include/rocksdb/options.h @@ -1843,8 +1843,11 @@ enum class BlobGarbageCollectionPolicy { // CompactRangeOptions is used by CompactRange() call. struct CompactRangeOptions { // If true, no other compaction will run at the same time as this - // manual compaction - bool exclusive_manual_compaction = true; + // manual compaction. + // + // Default: false + bool exclusive_manual_compaction = false; + // If true, compacted files will be moved to the minimum level capable // of holding the data or given level (specified non-negative target_level). bool change_level = false;