From 5620efc794f4b7f9747016d45d6efb6ed4607b9b Mon Sep 17 00:00:00 2001 From: Changyu Bi Date: Fri, 2 Feb 2024 17:09:42 -0800 Subject: [PATCH] Remove deprecated option `ignore_max_compaction_bytes_for_input` (#12323) Summary: The option is introduced in https://github.com/facebook/rocksdb/issues/10835 to allow disabling the new compaction behavior if it's not safe. The option is enabled by default and there has not been a need to disable it. So it should be safe to remove now. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12323 Reviewed By: ajkr Differential Revision: D53330336 Pulled By: cbi42 fbshipit-source-id: 36eef4664ac96b3a7ed627c48bd6610b0a7eafc5 --- db/compaction/compaction_picker.cc | 16 ++---- db/compaction/compaction_picker.h | 1 - db/compaction/compaction_picker_level.cc | 5 +- db/compaction/compaction_picker_test.cc | 55 +++++-------------- db/compaction/compaction_picker_universal.cc | 8 +-- db/compaction/tiered_compaction_test.cc | 1 - include/rocksdb/advanced_options.h | 13 ----- options/cf_options.cc | 6 +- options/cf_options.h | 4 -- options/options.cc | 4 -- options/options_helper.cc | 2 - ...e_ignore_max_compaction_bytes_for_input.md | 1 + 12 files changed, 27 insertions(+), 89 deletions(-) create mode 100644 unreleased_history/public_api_changes/remove_ignore_max_compaction_bytes_for_input.md diff --git a/db/compaction/compaction_picker.cc b/db/compaction/compaction_picker.cc index 4d40ab5034..67e8ac0633 100644 --- a/db/compaction/compaction_picker.cc +++ b/db/compaction/compaction_picker.cc @@ -456,10 +456,9 @@ bool CompactionPicker::IsRangeInCompaction(VersionStorageInfo* vstorage, // Returns false if files on parent level are currently in compaction, which // means that we can't compact them bool CompactionPicker::SetupOtherInputs( - const std::string& cf_name, const MutableCFOptions& mutable_cf_options, - VersionStorageInfo* vstorage, CompactionInputFiles* inputs, - CompactionInputFiles* output_level_inputs, int* parent_index, - int base_index, bool only_expand_towards_right) { + const std::string& cf_name, VersionStorageInfo* vstorage, + CompactionInputFiles* inputs, CompactionInputFiles* output_level_inputs, + int* parent_index, int base_index, bool only_expand_towards_right) { assert(!inputs->empty()); assert(output_level_inputs->empty()); const int input_level = inputs->level; @@ -500,7 +499,6 @@ bool CompactionPicker::SetupOtherInputs( // user key, while excluding other entries for the same user key. This // can happen when one user key spans multiple files. if (!output_level_inputs->empty()) { - const uint64_t limit = mutable_cf_options.max_compaction_bytes; const uint64_t output_level_inputs_size = TotalFileSize(output_level_inputs->files); const uint64_t inputs_size = TotalFileSize(inputs->files); @@ -527,8 +525,6 @@ bool CompactionPicker::SetupOtherInputs( try_overlapping_inputs = false; } if (try_overlapping_inputs && expanded_inputs.size() > inputs->size() && - (mutable_cf_options.ignore_max_compaction_bytes_for_input || - output_level_inputs_size + expanded_inputs_size < limit) && !AreFilesInCompaction(expanded_inputs.files)) { InternalKey new_start, new_limit; GetRange(expanded_inputs, &new_start, &new_limit); @@ -551,8 +547,6 @@ bool CompactionPicker::SetupOtherInputs( base_index, nullptr); expanded_inputs_size = TotalFileSize(expanded_inputs.files); if (expanded_inputs.size() > inputs->size() && - (mutable_cf_options.ignore_max_compaction_bytes_for_input || - output_level_inputs_size + expanded_inputs_size < limit) && !AreFilesInCompaction(expanded_inputs.files)) { expand_inputs = true; } @@ -812,8 +806,8 @@ Compaction* CompactionPicker::CompactRange( output_level_inputs.level = output_level; if (input_level != output_level) { int parent_index = -1; - if (!SetupOtherInputs(cf_name, mutable_cf_options, vstorage, &inputs, - &output_level_inputs, &parent_index, -1)) { + if (!SetupOtherInputs(cf_name, vstorage, &inputs, &output_level_inputs, + &parent_index, -1)) { // manual compaction is now multi-threaded, so it can // happen that SetupOtherInputs fails // we handle it higher in RunManualCompaction diff --git a/db/compaction/compaction_picker.h b/db/compaction/compaction_picker.h index 0cec71b475..63542a387a 100644 --- a/db/compaction/compaction_picker.h +++ b/db/compaction/compaction_picker.h @@ -186,7 +186,6 @@ class CompactionPicker { int penultimate_level) const; bool SetupOtherInputs(const std::string& cf_name, - const MutableCFOptions& mutable_cf_options, VersionStorageInfo* vstorage, CompactionInputFiles* inputs, CompactionInputFiles* output_level_inputs, diff --git a/db/compaction/compaction_picker_level.cc b/db/compaction/compaction_picker_level.cc index 72df24f80d..328baa988c 100644 --- a/db/compaction/compaction_picker_level.cc +++ b/db/compaction/compaction_picker_level.cc @@ -465,9 +465,8 @@ bool LevelCompactionBuilder::SetupOtherInputsIfNeeded() { } if (!is_l0_trivial_move_ && !compaction_picker_->SetupOtherInputs( - cf_name_, mutable_cf_options_, vstorage_, &start_level_inputs_, - &output_level_inputs_, &parent_index_, base_index_, - round_robin_expanding)) { + cf_name_, vstorage_, &start_level_inputs_, &output_level_inputs_, + &parent_index_, base_index_, round_robin_expanding)) { return false; } diff --git a/db/compaction/compaction_picker_test.cc b/db/compaction/compaction_picker_test.cc index 9d129be938..bde61a3667 100644 --- a/db/compaction/compaction_picker_test.cc +++ b/db/compaction/compaction_picker_test.cc @@ -1443,15 +1443,12 @@ TEST_F(CompactionPickerTest, CompactionPriMinOverlapping4) { ioptions_.compaction_pri = kMinOverlappingRatio; mutable_cf_options_.max_bytes_for_level_base = 10000000; mutable_cf_options_.max_bytes_for_level_multiplier = 10; - mutable_cf_options_.ignore_max_compaction_bytes_for_input = false; - // file 7 and 8 over lap with the same file, but file 8 is smaller so - // it will be picked. - // Overlaps with file 26, 27. And the file is compensated so will be - // picked up. + // Overlaps with file 26, 27, ratio is + // (60000000U + 60000000U / 180000000U) = 0.67 Add(2, 6U, "150", "167", 60000000U, 0, 100, 100, 180000000U); - Add(2, 7U, "168", "169", 60000000U); // Overlaps with file 27 - Add(2, 8U, "201", "300", 61000000U); // Overlaps with file 28 + Add(2, 7U, "178", "189", 60000000U); // Overlaps with file 28 + Add(2, 8U, "401", "500", 61000000U); // Overlaps with file 29 Add(3, 26U, "160", "165", 60000000U); // Boosted file size in output level is not considered. @@ -1465,7 +1462,7 @@ TEST_F(CompactionPickerTest, CompactionPriMinOverlapping4) { &log_buffer_)); ASSERT_TRUE(compaction.get() != nullptr); ASSERT_EQ(1U, compaction->num_input_files(0)); - // Picking file 8 because overlapping ratio is the biggest. + // Picking file 6 because overlapping ratio is the biggest. ASSERT_EQ(6U, compaction->input(0, 0)->fd.GetNumber()); } @@ -2468,20 +2465,23 @@ TEST_F(CompactionPickerTest, IsBottommostLevelTest) { DeleteVersionStorage(); } -TEST_F(CompactionPickerTest, MaxCompactionBytesHit) { +TEST_F(CompactionPickerTest, IgnoreCompactionLimitWhenAddFileFromInputLevel) { mutable_cf_options_.max_bytes_for_level_base = 1000000u; mutable_cf_options_.max_compaction_bytes = 800000u; - mutable_cf_options_.ignore_max_compaction_bytes_for_input = false; ioptions_.level_compaction_dynamic_level_bytes = false; NewVersionStorage(6, kCompactionStyleLevel); // A compaction should be triggered and pick file 2 and 5. - // It can expand because adding file 1 and 3, the compaction size will - // exceed mutable_cf_options_.max_bytes_for_level_base. + // It pulls in other compaction input file from the input level L1 + // without pulling in more output level files. + // Files 1, 3, 4 will be included in the compaction. + // File 6 is excluded since it overlaps with file 7. Add(1, 1U, "100", "150", 300000U); Add(1, 2U, "151", "200", 300001U, 0, 0); Add(1, 3U, "201", "250", 300000U, 0, 0); Add(1, 4U, "251", "300", 300000U, 0, 0); - Add(2, 5U, "100", "256", 1U); + Add(1, 6U, "325", "400", 300000U, 0, 0); + Add(2, 5U, "100", "350", 1U); + Add(2, 7U, "375", "425", 1U); UpdateVersionStorageInfo(); std::unique_ptr compaction(level_compaction_picker.PickCompaction( @@ -2489,37 +2489,12 @@ TEST_F(CompactionPickerTest, MaxCompactionBytesHit) { &log_buffer_)); ASSERT_TRUE(compaction.get() != nullptr); ASSERT_EQ(2U, compaction->num_input_levels()); - ASSERT_EQ(1U, compaction->num_input_files(0)); - ASSERT_EQ(1U, compaction->num_input_files(1)); - ASSERT_EQ(2U, compaction->input(0, 0)->fd.GetNumber()); - ASSERT_EQ(5U, compaction->input(1, 0)->fd.GetNumber()); -} - -TEST_F(CompactionPickerTest, MaxCompactionBytesNotHit) { - mutable_cf_options_.max_bytes_for_level_base = 800000u; - mutable_cf_options_.max_compaction_bytes = 1000000u; - mutable_cf_options_.ignore_max_compaction_bytes_for_input = false; - ioptions_.level_compaction_dynamic_level_bytes = false; - NewVersionStorage(6, kCompactionStyleLevel); - // A compaction should be triggered and pick file 2 and 5. - // and it expands to file 1 and 3 too. - Add(1, 1U, "100", "150", 300000U); - Add(1, 2U, "151", "200", 300001U, 0, 0); - Add(1, 3U, "201", "250", 300000U, 0, 0); - Add(1, 4U, "251", "300", 300000U, 0, 0); - Add(2, 5U, "000", "251", 1U); - UpdateVersionStorageInfo(); - - std::unique_ptr compaction(level_compaction_picker.PickCompaction( - cf_name_, mutable_cf_options_, mutable_db_options_, vstorage_.get(), - &log_buffer_)); - ASSERT_TRUE(compaction.get() != nullptr); - ASSERT_EQ(2U, compaction->num_input_levels()); - ASSERT_EQ(3U, compaction->num_input_files(0)); + ASSERT_EQ(4U, compaction->num_input_files(0)); ASSERT_EQ(1U, compaction->num_input_files(1)); ASSERT_EQ(1U, compaction->input(0, 0)->fd.GetNumber()); ASSERT_EQ(2U, compaction->input(0, 1)->fd.GetNumber()); ASSERT_EQ(3U, compaction->input(0, 2)->fd.GetNumber()); + ASSERT_EQ(4U, compaction->input(0, 3)->fd.GetNumber()); ASSERT_EQ(5U, compaction->input(1, 0)->fd.GetNumber()); } diff --git a/db/compaction/compaction_picker_universal.cc b/db/compaction/compaction_picker_universal.cc index 6d9ff43cd5..ae0ec4d17c 100644 --- a/db/compaction/compaction_picker_universal.cc +++ b/db/compaction/compaction_picker_universal.cc @@ -1139,8 +1139,7 @@ Compaction* UniversalCompactionBuilder::PickIncrementalForReduceSizeAmp( // from bottom_start_idx and bottom_end_idx, but for now, we use // SetupOtherInputs() for simplicity. int parent_index = -1; // Create and use bottom_start_idx? - if (!picker_->SetupOtherInputs(cf_name_, mutable_cf_options_, vstorage_, - &second_last_level_inputs, + if (!picker_->SetupOtherInputs(cf_name_, vstorage_, &second_last_level_inputs, &bottom_level_inputs, &parent_index, /*base_index=*/-1)) { return nullptr; @@ -1311,9 +1310,8 @@ Compaction* UniversalCompactionBuilder::PickDeleteTriggeredCompaction() { int parent_index = -1; output_level_inputs.level = output_level; - if (!picker_->SetupOtherInputs(cf_name_, mutable_cf_options_, vstorage_, - &start_level_inputs, &output_level_inputs, - &parent_index, -1)) { + if (!picker_->SetupOtherInputs(cf_name_, vstorage_, &start_level_inputs, + &output_level_inputs, &parent_index, -1)) { return nullptr; } inputs.push_back(start_level_inputs); diff --git a/db/compaction/tiered_compaction_test.cc b/db/compaction/tiered_compaction_test.cc index 15bc75b94c..f90c759c15 100644 --- a/db/compaction/tiered_compaction_test.cc +++ b/db/compaction/tiered_compaction_test.cc @@ -1823,7 +1823,6 @@ TEST_P(PrecludeLastLevelTestWithParms, PeriodicCompactionToPenultimateLevel) { options.env = mock_env_.get(); options.level0_file_num_compaction_trigger = kNumTrigger; options.num_levels = kNumLevels; - options.ignore_max_compaction_bytes_for_input = false; options.periodic_compaction_seconds = 10000; DestroyAndReopen(options); diff --git a/include/rocksdb/advanced_options.h b/include/rocksdb/advanced_options.h index 65572976f1..0c1f52f676 100644 --- a/include/rocksdb/advanced_options.h +++ b/include/rocksdb/advanced_options.h @@ -573,19 +573,6 @@ struct AdvancedColumnFamilyOptions { // Dynamically changeable through SetOptions() API uint64_t max_compaction_bytes = 0; - // DEPRECATED: This option might be removed in a future release. - // - // When setting up compaction input files, we ignore the - // `max_compaction_bytes` limit when pulling in input files that are entirely - // within output key range. - // - // Default: true - // - // Dynamically changeable through SetOptions() API - // We could remove this knob and always ignore the limit once it is proven - // safe. - bool ignore_max_compaction_bytes_for_input = true; - // All writes will be slowed down to at least delayed_write_rate if estimated // bytes needed to be compaction exceed this threshold. // diff --git a/options/cf_options.cc b/options/cf_options.cc index f17c193d3d..374522c21a 100644 --- a/options/cf_options.cc +++ b/options/cf_options.cc @@ -296,9 +296,7 @@ static std::unordered_map OptionType::kUInt64T, OptionVerificationType::kNormal, OptionTypeFlags::kMutable}}, {"ignore_max_compaction_bytes_for_input", - {offsetof(struct MutableCFOptions, - ignore_max_compaction_bytes_for_input), - OptionType::kBoolean, OptionVerificationType::kNormal, + {0, OptionType::kBoolean, OptionVerificationType::kDeprecated, OptionTypeFlags::kMutable}}, {"expanded_compaction_factor", {0, OptionType::kInt, OptionVerificationType::kDeprecated, @@ -1076,8 +1074,6 @@ void MutableCFOptions::Dump(Logger* log) const { level0_stop_writes_trigger); ROCKS_LOG_INFO(log, " max_compaction_bytes: %" PRIu64, max_compaction_bytes); - ROCKS_LOG_INFO(log, " ignore_max_compaction_bytes_for_input: %s", - ignore_max_compaction_bytes_for_input ? "true" : "false"); ROCKS_LOG_INFO(log, " target_file_size_base: %" PRIu64, target_file_size_base); ROCKS_LOG_INFO(log, " target_file_size_multiplier: %d", diff --git a/options/cf_options.h b/options/cf_options.h index a3fdfeea91..c27c81a456 100644 --- a/options/cf_options.h +++ b/options/cf_options.h @@ -132,8 +132,6 @@ struct MutableCFOptions { level0_slowdown_writes_trigger(options.level0_slowdown_writes_trigger), level0_stop_writes_trigger(options.level0_stop_writes_trigger), max_compaction_bytes(options.max_compaction_bytes), - ignore_max_compaction_bytes_for_input( - options.ignore_max_compaction_bytes_for_input), target_file_size_base(options.target_file_size_base), target_file_size_multiplier(options.target_file_size_multiplier), max_bytes_for_level_base(options.max_bytes_for_level_base), @@ -198,7 +196,6 @@ struct MutableCFOptions { level0_slowdown_writes_trigger(0), level0_stop_writes_trigger(0), max_compaction_bytes(0), - ignore_max_compaction_bytes_for_input(true), target_file_size_base(0), target_file_size_multiplier(0), max_bytes_for_level_base(0), @@ -281,7 +278,6 @@ struct MutableCFOptions { int level0_slowdown_writes_trigger; int level0_stop_writes_trigger; uint64_t max_compaction_bytes; - bool ignore_max_compaction_bytes_for_input; uint64_t target_file_size_base; int target_file_size_multiplier; uint64_t max_bytes_for_level_base; diff --git a/options/options.cc b/options/options.cc index 9690862b44..e20f6b0fb2 100644 --- a/options/options.cc +++ b/options/options.cc @@ -71,8 +71,6 @@ AdvancedColumnFamilyOptions::AdvancedColumnFamilyOptions(const Options& options) max_bytes_for_level_multiplier_additional( options.max_bytes_for_level_multiplier_additional), max_compaction_bytes(options.max_compaction_bytes), - ignore_max_compaction_bytes_for_input( - options.ignore_max_compaction_bytes_for_input), soft_pending_compaction_bytes_limit( options.soft_pending_compaction_bytes_limit), hard_pending_compaction_bytes_limit( @@ -290,8 +288,6 @@ void ColumnFamilyOptions::Dump(Logger* log) const { ROCKS_LOG_HEADER( log, " Options.max_compaction_bytes: %" PRIu64, max_compaction_bytes); - ROCKS_LOG_HEADER(log, " Options.ignore_max_compaction_bytes_for_input: %s", - ignore_max_compaction_bytes_for_input ? "true" : "false"); ROCKS_LOG_HEADER( log, " Options.arena_block_size: %" ROCKSDB_PRIszt, diff --git a/options/options_helper.cc b/options/options_helper.cc index 088a6e6208..8cc671264f 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -229,8 +229,6 @@ void UpdateColumnFamilyOptions(const MutableCFOptions& moptions, moptions.level0_slowdown_writes_trigger; cf_opts->level0_stop_writes_trigger = moptions.level0_stop_writes_trigger; cf_opts->max_compaction_bytes = moptions.max_compaction_bytes; - cf_opts->ignore_max_compaction_bytes_for_input = - moptions.ignore_max_compaction_bytes_for_input; cf_opts->target_file_size_base = moptions.target_file_size_base; cf_opts->target_file_size_multiplier = moptions.target_file_size_multiplier; cf_opts->max_bytes_for_level_base = moptions.max_bytes_for_level_base; diff --git a/unreleased_history/public_api_changes/remove_ignore_max_compaction_bytes_for_input.md b/unreleased_history/public_api_changes/remove_ignore_max_compaction_bytes_for_input.md new file mode 100644 index 0000000000..5e4d12a72c --- /dev/null +++ b/unreleased_history/public_api_changes/remove_ignore_max_compaction_bytes_for_input.md @@ -0,0 +1 @@ +Removed deprecated option `ColumnFamilyOptions::ignore_max_compaction_bytes_for_input`