diff --git a/db/builder.cc b/db/builder.cc index 54749b6893..d754d49559 100644 --- a/db/builder.cc +++ b/db/builder.cc @@ -83,11 +83,8 @@ Status BuildTable( auto& ioptions = tboptions.ioptions; // Reports the IOStats for flush for every following bytes. const size_t kReportFlushIOStatsEvery = 1048576; - OutputValidator output_validator( - tboptions.internal_comparator, - /*enable_order_check=*/ - mutable_cf_options.check_flush_compaction_key_order, - /*enable_hash=*/paranoid_file_checks); + OutputValidator output_validator(tboptions.internal_comparator, + /*enable_hash=*/paranoid_file_checks); Status s; meta->fd.file_size = 0; iter->SeekToFirst(); @@ -425,7 +422,6 @@ Status BuildTable( s = it->status(); if (s.ok() && paranoid_file_checks) { OutputValidator file_validator(tboptions.internal_comparator, - /*enable_order_check=*/true, /*enable_hash=*/true); for (it->SeekToFirst(); it->Valid(); it->Next()) { // Generate a rolling 64-bit hash of the key and values diff --git a/db/compaction/compaction_job.cc b/db/compaction/compaction_job.cc index 9dc4e31b9a..867f8090dc 100644 --- a/db/compaction/compaction_job.cc +++ b/db/compaction/compaction_job.cc @@ -760,7 +760,6 @@ Status CompactionJob::Run() { if (s.ok() && paranoid_file_checks_) { OutputValidator validator(cfd->internal_comparator(), - /*_enable_order_check=*/true, /*_enable_hash=*/true); for (iter->SeekToFirst(); iter->Valid(); iter->Next()) { s = validator.Add(iter->key(), iter->value()); @@ -1948,8 +1947,6 @@ Status CompactionJob::OpenCompactionOutputFile(SubcompactionState* sub_compact, } outputs.AddOutput(std::move(meta), cfd->internal_comparator(), - sub_compact->compaction->mutable_cf_options() - ->check_flush_compaction_key_order, paranoid_file_checks_); } diff --git a/db/compaction/compaction_outputs.h b/db/compaction/compaction_outputs.h index 18246cf2fa..f232214e3b 100644 --- a/db/compaction/compaction_outputs.h +++ b/db/compaction/compaction_outputs.h @@ -30,11 +30,9 @@ class CompactionOutputs { // compaction output file struct Output { Output(FileMetaData&& _meta, const InternalKeyComparator& _icmp, - bool _enable_order_check, bool _enable_hash, bool _finished, - uint64_t precalculated_hash) + bool _enable_hash, bool _finished, uint64_t precalculated_hash) : meta(std::move(_meta)), - validator(_icmp, _enable_order_check, _enable_hash, - precalculated_hash), + validator(_icmp, _enable_hash, precalculated_hash), finished(_finished) {} FileMetaData meta; OutputValidator validator; @@ -49,10 +47,10 @@ class CompactionOutputs { // Add generated output to the list void AddOutput(FileMetaData&& meta, const InternalKeyComparator& icmp, - bool enable_order_check, bool enable_hash, - bool finished = false, uint64_t precalculated_hash = 0) { - outputs_.emplace_back(std::move(meta), icmp, enable_order_check, - enable_hash, finished, precalculated_hash); + bool enable_hash, bool finished = false, + uint64_t precalculated_hash = 0) { + outputs_.emplace_back(std::move(meta), icmp, enable_hash, finished, + precalculated_hash); } // Set new table builder for the current output diff --git a/db/compaction/compaction_service_job.cc b/db/compaction/compaction_service_job.cc index 3149bb5002..442eaf8ea7 100644 --- a/db/compaction/compaction_service_job.cc +++ b/db/compaction/compaction_service_job.cc @@ -195,8 +195,8 @@ CompactionJob::ProcessKeyValueCompactionWithCompactionService( auto cfd = compaction->column_family_data(); sub_compact->Current().AddOutput(std::move(meta), - cfd->internal_comparator(), false, false, - true, file.paranoid_hash); + cfd->internal_comparator(), false, true, + file.paranoid_hash); } sub_compact->compaction_job_stats = compaction_result.stats; sub_compact->Current().SetNumOutputRecords( diff --git a/db/corruption_test.cc b/db/corruption_test.cc index 4e6a823687..e9e7344fd1 100644 --- a/db/corruption_test.cc +++ b/db/corruption_test.cc @@ -822,7 +822,6 @@ TEST_F(CorruptionTest, ParanoidFileChecksOnFlush) { Options options; options.level_compaction_dynamic_level_bytes = false; options.env = env_.get(); - options.check_flush_compaction_key_order = false; options.paranoid_file_checks = true; options.create_if_missing = true; Status s; @@ -853,7 +852,6 @@ TEST_F(CorruptionTest, ParanoidFileChecksOnCompact) { options.env = env_.get(); options.paranoid_file_checks = true; options.create_if_missing = true; - options.check_flush_compaction_key_order = false; Status s; for (const auto& mode : corruption_modes) { delete db_; @@ -885,7 +883,6 @@ TEST_F(CorruptionTest, ParanoidFileChecksWithDeleteRangeFirst) { Options options; options.level_compaction_dynamic_level_bytes = false; options.env = env_.get(); - options.check_flush_compaction_key_order = false; options.paranoid_file_checks = true; options.create_if_missing = true; for (bool do_flush : {true, false}) { @@ -922,7 +919,6 @@ TEST_F(CorruptionTest, ParanoidFileChecksWithDeleteRange) { Options options; options.level_compaction_dynamic_level_bytes = false; options.env = env_.get(); - options.check_flush_compaction_key_order = false; options.paranoid_file_checks = true; options.create_if_missing = true; for (bool do_flush : {true, false}) { @@ -962,7 +958,6 @@ TEST_F(CorruptionTest, ParanoidFileChecksWithDeleteRangeLast) { Options options; options.level_compaction_dynamic_level_bytes = false; options.env = env_.get(); - options.check_flush_compaction_key_order = false; options.paranoid_file_checks = true; options.create_if_missing = true; for (bool do_flush : {true, false}) { @@ -1031,7 +1026,6 @@ TEST_F(CorruptionTest, CompactionKeyOrderCheck) { options.env = env_.get(); options.paranoid_file_checks = false; options.create_if_missing = true; - options.check_flush_compaction_key_order = false; delete db_; db_ = nullptr; ASSERT_OK(DestroyDB(dbname_, options)); @@ -1046,7 +1040,6 @@ TEST_F(CorruptionTest, CompactionKeyOrderCheck) { ASSERT_OK(dbi->TEST_FlushMemTable()); mock->SetCorruptionMode(mock::MockTableFactory::kCorruptNone); - ASSERT_OK(db_->SetOptions({{"check_flush_compaction_key_order", "true"}})); CompactRangeOptions cro; cro.bottommost_level_compaction = BottommostLevelCompaction::kForce; ASSERT_NOK( @@ -1059,7 +1052,6 @@ TEST_F(CorruptionTest, FlushKeyOrderCheck) { options.env = env_.get(); options.paranoid_file_checks = false; options.create_if_missing = true; - ASSERT_OK(db_->SetOptions({{"check_flush_compaction_key_order", "true"}})); ASSERT_OK(db_->Put(WriteOptions(), "foo1", "v1")); ASSERT_OK(db_->Put(WriteOptions(), "foo2", "v1")); @@ -1084,28 +1076,6 @@ TEST_F(CorruptionTest, FlushKeyOrderCheck) { ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->ClearAllCallBacks(); } -TEST_F(CorruptionTest, DisableKeyOrderCheck) { - ASSERT_OK(db_->SetOptions({{"check_flush_compaction_key_order", "false"}})); - DBImpl* dbi = static_cast_with_check(db_); - - SyncPoint::GetInstance()->SetCallBack( - "OutputValidator::Add:order_check", - [&](void* /*arg*/) { ASSERT_TRUE(false); }); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing(); - ASSERT_OK(db_->Put(WriteOptions(), "foo1", "v1")); - ASSERT_OK(db_->Put(WriteOptions(), "foo3", "v1")); - ASSERT_OK(dbi->TEST_FlushMemTable()); - ASSERT_OK(db_->Put(WriteOptions(), "foo2", "v1")); - ASSERT_OK(db_->Put(WriteOptions(), "foo4", "v1")); - ASSERT_OK(dbi->TEST_FlushMemTable()); - CompactRangeOptions cro; - cro.bottommost_level_compaction = BottommostLevelCompaction::kForce; - ASSERT_OK( - dbi->CompactRange(cro, dbi->DefaultColumnFamily(), nullptr, nullptr)); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing(); - ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->ClearAllCallBacks(); -} - TEST_F(CorruptionTest, VerifyWholeTableChecksum) { CloseDb(); Options options; diff --git a/db/db_test.cc b/db/db_test.cc index 4acc744f08..b47d706fe5 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -5813,13 +5813,6 @@ TEST_F(DBTest, DynamicMiscOptions) { ASSERT_OK(dbfull()->TEST_GetLatestMutableCFOptions(handles_[1], &mutable_cf_options)); ASSERT_TRUE(mutable_cf_options.report_bg_io_stats); - ASSERT_TRUE(mutable_cf_options.check_flush_compaction_key_order); - - ASSERT_OK(dbfull()->SetOptions( - handles_[1], {{"check_flush_compaction_key_order", "false"}})); - ASSERT_OK(dbfull()->TEST_GetLatestMutableCFOptions(handles_[1], - &mutable_cf_options)); - ASSERT_FALSE(mutable_cf_options.check_flush_compaction_key_order); } TEST_F(DBTest, L0L1L2AndUpHitCounter) { diff --git a/db/output_validator.cc b/db/output_validator.cc index e93e2d68c4..0c7109f3c2 100644 --- a/db/output_validator.cc +++ b/db/output_validator.cc @@ -15,19 +15,15 @@ Status OutputValidator::Add(const Slice& key, const Slice& value) { paranoid_hash_ = NPHash64(key.data(), key.size(), paranoid_hash_); paranoid_hash_ = NPHash64(value.data(), value.size(), paranoid_hash_); } - if (enable_order_check_) { - TEST_SYNC_POINT_CALLBACK("OutputValidator::Add:order_check", - /*arg=*/nullptr); - if (key.size() < kNumInternalBytes) { - return Status::Corruption( - "Compaction tries to write a key without internal bytes."); - } - // prev_key_ starts with empty. - if (!prev_key_.empty() && icmp_.Compare(key, prev_key_) < 0) { - return Status::Corruption("Compaction sees out-of-order keys."); - } - prev_key_.assign(key.data(), key.size()); + if (key.size() < kNumInternalBytes) { + return Status::Corruption( + "Compaction tries to write a key without internal bytes."); } + // prev_key_ starts with empty. + if (!prev_key_.empty() && icmp_.Compare(key, prev_key_) < 0) { + return Status::Corruption("Compaction sees out-of-order keys."); + } + prev_key_.assign(key.data(), key.size()); return Status::OK(); } } // namespace ROCKSDB_NAMESPACE diff --git a/db/output_validator.h b/db/output_validator.h index 40635f9c44..1e7a8988ed 100644 --- a/db/output_validator.h +++ b/db/output_validator.h @@ -15,12 +15,10 @@ namespace ROCKSDB_NAMESPACE { // of all the key and value. class OutputValidator { public: - explicit OutputValidator(const InternalKeyComparator& icmp, - bool enable_order_check, bool enable_hash, + explicit OutputValidator(const InternalKeyComparator& icmp, bool enable_hash, uint64_t precalculated_hash = 0) : icmp_(icmp), paranoid_hash_(precalculated_hash), - enable_order_check_(enable_order_check), enable_hash_(enable_hash) {} // Add a key to the KV sequence, and return whether the key follows @@ -42,7 +40,6 @@ class OutputValidator { const InternalKeyComparator& icmp_; std::string prev_key_; uint64_t paranoid_hash_ = 0; - bool enable_order_check_; bool enable_hash_; }; } // namespace ROCKSDB_NAMESPACE diff --git a/include/rocksdb/advanced_options.h b/include/rocksdb/advanced_options.h index fd51e22657..a5c2eba7ef 100644 --- a/include/rocksdb/advanced_options.h +++ b/include/rocksdb/advanced_options.h @@ -701,16 +701,6 @@ struct AdvancedColumnFamilyOptions { // Default: false bool optimize_filters_for_hits = false; - // DEPRECATED: This option might be removed in a future release. - // - // During flush or compaction, check whether keys inserted to output files - // are in order. - // - // Default: true - // - // Dynamically changeable through SetOptions() API - bool check_flush_compaction_key_order = true; - // After writing every SST file, reopen it and read all the keys. // Checks the hash of all of the keys and values written versus the // keys in the file and signals a corruption if they do not match diff --git a/options/cf_options.cc b/options/cf_options.cc index cad6e2ac09..e4b5a7d3a2 100644 --- a/options/cf_options.cc +++ b/options/cf_options.cc @@ -266,8 +266,7 @@ static std::unordered_map {0, OptionType::kBoolean, OptionVerificationType::kDeprecated, OptionTypeFlags::kMutable}}, {"check_flush_compaction_key_order", - {offsetof(struct MutableCFOptions, check_flush_compaction_key_order), - OptionType::kBoolean, OptionVerificationType::kNormal, + {0, OptionType::kBoolean, OptionVerificationType::kDeprecated, OptionTypeFlags::kMutable}}, {"paranoid_file_checks", {offsetof(struct MutableCFOptions, paranoid_file_checks), @@ -1111,8 +1110,6 @@ void MutableCFOptions::Dump(Logger* log) const { result.c_str()); ROCKS_LOG_INFO(log, " max_sequential_skip_in_iterations: %" PRIu64, max_sequential_skip_in_iterations); - ROCKS_LOG_INFO(log, " check_flush_compaction_key_order: %d", - check_flush_compaction_key_order); ROCKS_LOG_INFO(log, " paranoid_file_checks: %d", paranoid_file_checks); ROCKS_LOG_INFO(log, " report_bg_io_stats: %d", diff --git a/options/cf_options.h b/options/cf_options.h index f42d6b5629..f6dea50316 100644 --- a/options/cf_options.h +++ b/options/cf_options.h @@ -160,8 +160,6 @@ struct MutableCFOptions { prepopulate_blob_cache(options.prepopulate_blob_cache), max_sequential_skip_in_iterations( options.max_sequential_skip_in_iterations), - check_flush_compaction_key_order( - options.check_flush_compaction_key_order), paranoid_file_checks(options.paranoid_file_checks), report_bg_io_stats(options.report_bg_io_stats), compression(options.compression), @@ -221,7 +219,6 @@ struct MutableCFOptions { blob_file_starting_level(0), prepopulate_blob_cache(PrepopulateBlobCache::kDisable), max_sequential_skip_in_iterations(0), - check_flush_compaction_key_order(true), paranoid_file_checks(false), report_bg_io_stats(false), compression(Snappy_Supported() ? kSnappyCompression : kNoCompression), @@ -311,7 +308,6 @@ struct MutableCFOptions { // Misc options uint64_t max_sequential_skip_in_iterations; - bool check_flush_compaction_key_order; bool paranoid_file_checks; bool report_bg_io_stats; CompressionType compression; diff --git a/options/options.cc b/options/options.cc index 2e7c025030..9690862b44 100644 --- a/options/options.cc +++ b/options/options.cc @@ -533,7 +533,6 @@ Options* Options::DisableExtraChecks() { // See https://github.com/facebook/rocksdb/issues/9354 force_consistency_checks = false; // Considered but no clear performance impact seen: - // * check_flush_compaction_key_order // * paranoid_checks // * flush_verify_memtable_count // By current API contract, not including diff --git a/options/options_helper.cc b/options/options_helper.cc index fa5d549c18..99895680f5 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -266,8 +266,6 @@ void UpdateColumnFamilyOptions(const MutableCFOptions& moptions, // Misc options cf_opts->max_sequential_skip_in_iterations = moptions.max_sequential_skip_in_iterations; - cf_opts->check_flush_compaction_key_order = - moptions.check_flush_compaction_key_order; cf_opts->paranoid_file_checks = moptions.paranoid_file_checks; cf_opts->report_bg_io_stats = moptions.report_bg_io_stats; cf_opts->compression = moptions.compression; diff --git a/tools/db_bench_tool.cc b/tools/db_bench_tool.cc index 3bfbf685b3..29fe3f8e1f 100644 --- a/tools/db_bench_tool.cc +++ b/tools/db_bench_tool.cc @@ -930,11 +930,6 @@ DEFINE_bool(force_consistency_checks, "Runs consistency checks on the LSM every time a change is " "applied."); -DEFINE_bool(check_flush_compaction_key_order, - ROCKSDB_NAMESPACE::Options().check_flush_compaction_key_order, - "During flush or compaction, check whether keys inserted to " - "output files are in order."); - DEFINE_uint64(delete_obsolete_files_period_micros, 0, "Ignored. Left here for backward compatibility"); @@ -4617,8 +4612,6 @@ class Benchmark { options.optimize_filters_for_hits = FLAGS_optimize_filters_for_hits; options.paranoid_checks = FLAGS_paranoid_checks; options.force_consistency_checks = FLAGS_force_consistency_checks; - options.check_flush_compaction_key_order = - FLAGS_check_flush_compaction_key_order; options.periodic_compaction_seconds = FLAGS_periodic_compaction_seconds; options.ttl = FLAGS_ttl_seconds; // fill storage options diff --git a/unreleased_history/public_api_changes/remove_check_flush_compaction_key_order.md b/unreleased_history/public_api_changes/remove_check_flush_compaction_key_order.md new file mode 100644 index 0000000000..d06559bf13 --- /dev/null +++ b/unreleased_history/public_api_changes/remove_check_flush_compaction_key_order.md @@ -0,0 +1 @@ +Removed deprecated option `ColumnFamilyOptions::check_flush_compaction_key_order`