dolysis
/
rocksdb
mirror of https://github.com/facebook/rocksdb.git
3
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Removed `check_flush_compaction_key_order` (#12311) 

Summary:
`check_flush_compaction_key_order` option was introduced for the key order checking online validation. It gave users the ability to disable the validation without downgrade in case the validation caused inefficiencies or false positives. Over time this validation has shown to be cheap and correct, so the option to disable it can now be removed.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/12311

Reviewed By: cbi42

Differential Revision: D53233379

Pulled By: ajkr

fbshipit-source-id: 1384361104021d6e3e580dce2ec123f9f99ce637
This commit is contained in:
Andrew Kryczka 2024-01-31 16:30:26 -08:00 committed by Facebook GitHub Bot
parent 76c834e441
commit f9d45358ca
15 changed files with 21 additions and 100 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
db/builder.cc
View File

@ -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

3
db/compaction/compaction_job.cc
View File

@ -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_);
}

14
db/compaction/compaction_outputs.h
View File

@ -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

4
db/compaction/compaction_service_job.cc
View File

@ -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(

30
db/corruption_test.cc
View File

@ -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<DBImpl>(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;

7
db/db_test.cc
View File

@ -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) {

20
db/output_validator.cc
View File

@ -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

5
db/output_validator.h
View File

@ -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

10
include/rocksdb/advanced_options.h
View File

@ -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

5
options/cf_options.cc
View File

@ -266,8 +266,7 @@ static std::unordered_map<std::string, OptionTypeInfo>
{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",

4
options/cf_options.h
View File

@ -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;

1
options/options.cc
View File

@ -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

2
options/options_helper.cc
View File

@ -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;

7
tools/db_bench_tool.cc
View File

@ -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

1
unreleased_history/public_api_changes/remove_check_flush_compaction_key_order.md Normal file
View File

@ -0,0 +1 @@
Removed deprecated option `ColumnFamilyOptions::check_flush_compaction_key_order`