From 648fe25bc02900266658a89987ed66fe092b7f5e Mon Sep 17 00:00:00 2001 From: Changyu Bi Date: Thu, 12 Oct 2023 15:26:10 -0700 Subject: [PATCH] Always clear files marked for compaction in `ComputeCompactionScore()` (#11946) Summary: We were seeing the following stress test failures: ```LevelCompactionBuilder::PickFileToCompact(const rocksdb::autovector >&, bool): Assertion `!level_file.second->being_compacted' failed``` This can happen when we are picking a file to be compacted from some files marked for compaction, but that file is already being_compacted. We prevent this by always calling `ComputeCompactionScore()` after we pick a compaction and mark some files as being_compacted. However, if SetOptions() is called to disable marking certain files to be compacted, say `enable_blob_garbage_collection`, we currently just skip the relevant logic in `ComputeCompactionScore()` without clearing the existing files already marked for compaction. This PR fixes this issue by already clearing these files. Pull Request resolved: https://github.com/facebook/rocksdb/pull/11946 Test Plan: existing tests. Reviewed By: akankshamahajan15 Differential Revision: D50232608 Pulled By: cbi42 fbshipit-source-id: 11e4fb5e9d48b0f946ad33b18f7c005f0161f496 --- db/db_impl/db_impl.cc | 3 ++- db/version_set.cc | 59 ++++++++++++++++++++++-------------------- db/version_set.h | 8 +++--- db/version_set_test.cc | 33 +++++++++++++++-------- 4 files changed, 60 insertions(+), 43 deletions(-) diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index 140d334ce4..3ae70b209b 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -4114,7 +4114,8 @@ void DBImpl::ReleaseSnapshot(const Snapshot* s) { CfdList cf_scheduled; for (auto* cfd : *versions_->GetColumnFamilySet()) { if (!cfd->ioptions()->allow_ingest_behind) { - cfd->current()->storage_info()->UpdateOldestSnapshot(oldest_snapshot); + cfd->current()->storage_info()->UpdateOldestSnapshot( + oldest_snapshot, /*allow_ingest_behind=*/false); if (!cfd->current() ->storage_info() ->BottommostFilesMarkedForCompaction() diff --git a/db/version_set.cc b/db/version_set.cc index 41e90e13db..95df9fb8d0 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -3582,26 +3582,16 @@ void VersionStorageInfo::ComputeCompactionScore( } } ComputeFilesMarkedForCompaction(max_output_level); - if (!immutable_options.allow_ingest_behind) { - ComputeBottommostFilesMarkedForCompaction(); - } - if (mutable_cf_options.ttl > 0 && - compaction_style_ == kCompactionStyleLevel) { - ComputeExpiredTtlFiles(immutable_options, mutable_cf_options.ttl); - } - if (mutable_cf_options.periodic_compaction_seconds > 0) { - ComputeFilesMarkedForPeriodicCompaction( - immutable_options, mutable_cf_options.periodic_compaction_seconds, - max_output_level); - } - - if (mutable_cf_options.enable_blob_garbage_collection && - mutable_cf_options.blob_garbage_collection_age_cutoff > 0.0 && - mutable_cf_options.blob_garbage_collection_force_threshold < 1.0) { - ComputeFilesMarkedForForcedBlobGC( - mutable_cf_options.blob_garbage_collection_age_cutoff, - mutable_cf_options.blob_garbage_collection_force_threshold); - } + ComputeBottommostFilesMarkedForCompaction( + immutable_options.allow_ingest_behind); + ComputeExpiredTtlFiles(immutable_options, mutable_cf_options.ttl); + ComputeFilesMarkedForPeriodicCompaction( + immutable_options, mutable_cf_options.periodic_compaction_seconds, + max_output_level); + ComputeFilesMarkedForForcedBlobGC( + mutable_cf_options.blob_garbage_collection_age_cutoff, + mutable_cf_options.blob_garbage_collection_force_threshold, + mutable_cf_options.enable_blob_garbage_collection); EstimateCompactionBytesNeeded(mutable_cf_options); } @@ -3631,9 +3621,10 @@ void VersionStorageInfo::ComputeFilesMarkedForCompaction(int last_level) { void VersionStorageInfo::ComputeExpiredTtlFiles( const ImmutableOptions& ioptions, const uint64_t ttl) { - assert(ttl > 0); - expired_ttl_files_.clear(); + if (ttl == 0 || compaction_style_ != CompactionStyle::kCompactionStyleLevel) { + return; + } int64_t _current_time; auto status = ioptions.clock->GetCurrentTime(&_current_time); @@ -3658,9 +3649,10 @@ void VersionStorageInfo::ComputeExpiredTtlFiles( void VersionStorageInfo::ComputeFilesMarkedForPeriodicCompaction( const ImmutableOptions& ioptions, const uint64_t periodic_compaction_seconds, int last_level) { - assert(periodic_compaction_seconds > 0); - files_marked_for_periodic_compaction_.clear(); + if (periodic_compaction_seconds == 0) { + return; + } int64_t temp_current_time; auto status = ioptions.clock->GetCurrentTime(&temp_current_time); @@ -3714,8 +3706,14 @@ void VersionStorageInfo::ComputeFilesMarkedForPeriodicCompaction( void VersionStorageInfo::ComputeFilesMarkedForForcedBlobGC( double blob_garbage_collection_age_cutoff, - double blob_garbage_collection_force_threshold) { + double blob_garbage_collection_force_threshold, + bool enable_blob_garbage_collection) { files_marked_for_forced_blob_gc_.clear(); + if (!(enable_blob_garbage_collection && + blob_garbage_collection_age_cutoff > 0.0 && + blob_garbage_collection_force_threshold < 1.0)) { + return; + } if (blob_files_.empty()) { return; @@ -4172,17 +4170,22 @@ void VersionStorageInfo::GenerateFileLocationIndex() { } } -void VersionStorageInfo::UpdateOldestSnapshot(SequenceNumber seqnum) { +void VersionStorageInfo::UpdateOldestSnapshot(SequenceNumber seqnum, + bool allow_ingest_behind) { assert(seqnum >= oldest_snapshot_seqnum_); oldest_snapshot_seqnum_ = seqnum; if (oldest_snapshot_seqnum_ > bottommost_files_mark_threshold_) { - ComputeBottommostFilesMarkedForCompaction(); + ComputeBottommostFilesMarkedForCompaction(allow_ingest_behind); } } -void VersionStorageInfo::ComputeBottommostFilesMarkedForCompaction() { +void VersionStorageInfo::ComputeBottommostFilesMarkedForCompaction( + bool allow_ingest_behind) { bottommost_files_marked_for_compaction_.clear(); bottommost_files_mark_threshold_ = kMaxSequenceNumber; + if (allow_ingest_behind) { + return; + } // If a file's creation time is larger than creation_time_ub, // it is too new to be marked for compaction. int64_t creation_time_ub = 0; diff --git a/db/version_set.h b/db/version_set.h index 6774cfcd11..bfc63e351e 100644 --- a/db/version_set.h +++ b/db/version_set.h @@ -228,7 +228,7 @@ class VersionStorageInfo { // eligible for compaction. // // REQUIRES: DB mutex held - void ComputeBottommostFilesMarkedForCompaction(); + void ComputeBottommostFilesMarkedForCompaction(bool allow_ingest_behind); // This computes files_marked_for_forced_blob_gc_ and is called by // ComputeCompactionScore() @@ -236,14 +236,16 @@ class VersionStorageInfo { // REQUIRES: DB mutex held void ComputeFilesMarkedForForcedBlobGC( double blob_garbage_collection_age_cutoff, - double blob_garbage_collection_force_threshold); + double blob_garbage_collection_force_threshold, + bool enable_blob_garbage_collection); bool level0_non_overlapping() const { return level0_non_overlapping_; } // Updates the oldest snapshot and related internal state, like the bottommost // files marked for compaction. // REQUIRES: DB mutex held - void UpdateOldestSnapshot(SequenceNumber oldest_snapshot_seqnum); + void UpdateOldestSnapshot(SequenceNumber oldest_snapshot_seqnum, + bool allow_ingest_behind); int MaxInputLevel() const; int MaxOutputLevel(bool allow_ingest_behind) const; diff --git a/db/version_set_test.cc b/db/version_set_test.cc index 59b9461512..2526e752f4 100644 --- a/db/version_set_test.cc +++ b/db/version_set_test.cc @@ -584,7 +584,8 @@ TEST_F(VersionStorageInfoTest, ForcedBlobGCEmpty) { constexpr double age_cutoff = 0.5; constexpr double force_threshold = 0.75; - vstorage_.ComputeFilesMarkedForForcedBlobGC(age_cutoff, force_threshold); + vstorage_.ComputeFilesMarkedForForcedBlobGC( + age_cutoff, force_threshold, /*enable_blob_garbage_collection=*/true); ASSERT_TRUE(vstorage_.FilesMarkedForForcedBlobGC().empty()); } @@ -668,7 +669,8 @@ TEST_F(VersionStorageInfoTest, ForcedBlobGCSingleBatch) { { constexpr double age_cutoff = 0.1; constexpr double force_threshold = 0.0; - vstorage_.ComputeFilesMarkedForForcedBlobGC(age_cutoff, force_threshold); + vstorage_.ComputeFilesMarkedForForcedBlobGC( + age_cutoff, force_threshold, /*enable_blob_garbage_collection=*/true); ASSERT_TRUE(vstorage_.FilesMarkedForForcedBlobGC().empty()); } @@ -679,7 +681,8 @@ TEST_F(VersionStorageInfoTest, ForcedBlobGCSingleBatch) { { constexpr double age_cutoff = 0.5; constexpr double force_threshold = 0.0; - vstorage_.ComputeFilesMarkedForForcedBlobGC(age_cutoff, force_threshold); + vstorage_.ComputeFilesMarkedForForcedBlobGC( + age_cutoff, force_threshold, /*enable_blob_garbage_collection=*/true); ASSERT_TRUE(vstorage_.FilesMarkedForForcedBlobGC().empty()); } @@ -690,7 +693,8 @@ TEST_F(VersionStorageInfoTest, ForcedBlobGCSingleBatch) { { constexpr double age_cutoff = 1.0; constexpr double force_threshold = 0.6; - vstorage_.ComputeFilesMarkedForForcedBlobGC(age_cutoff, force_threshold); + vstorage_.ComputeFilesMarkedForForcedBlobGC( + age_cutoff, force_threshold, /*enable_blob_garbage_collection=*/true); ASSERT_TRUE(vstorage_.FilesMarkedForForcedBlobGC().empty()); } @@ -701,7 +705,8 @@ TEST_F(VersionStorageInfoTest, ForcedBlobGCSingleBatch) { { constexpr double age_cutoff = 1.0; constexpr double force_threshold = 0.5; - vstorage_.ComputeFilesMarkedForForcedBlobGC(age_cutoff, force_threshold); + vstorage_.ComputeFilesMarkedForForcedBlobGC( + age_cutoff, force_threshold, /*enable_blob_garbage_collection=*/true); auto ssts_to_be_compacted = vstorage_.FilesMarkedForForcedBlobGC(); ASSERT_EQ(ssts_to_be_compacted.size(), 1); @@ -815,7 +820,8 @@ TEST_F(VersionStorageInfoTest, ForcedBlobGCMultipleBatches) { { constexpr double age_cutoff = 0.1; constexpr double force_threshold = 0.0; - vstorage_.ComputeFilesMarkedForForcedBlobGC(age_cutoff, force_threshold); + vstorage_.ComputeFilesMarkedForForcedBlobGC( + age_cutoff, force_threshold, /*enable_blob_garbage_collection=*/true); ASSERT_TRUE(vstorage_.FilesMarkedForForcedBlobGC().empty()); } @@ -826,7 +832,8 @@ TEST_F(VersionStorageInfoTest, ForcedBlobGCMultipleBatches) { { constexpr double age_cutoff = 0.25; constexpr double force_threshold = 0.0; - vstorage_.ComputeFilesMarkedForForcedBlobGC(age_cutoff, force_threshold); + vstorage_.ComputeFilesMarkedForForcedBlobGC( + age_cutoff, force_threshold, /*enable_blob_garbage_collection=*/true); ASSERT_TRUE(vstorage_.FilesMarkedForForcedBlobGC().empty()); } @@ -837,7 +844,8 @@ TEST_F(VersionStorageInfoTest, ForcedBlobGCMultipleBatches) { { constexpr double age_cutoff = 0.5; constexpr double force_threshold = 0.6; - vstorage_.ComputeFilesMarkedForForcedBlobGC(age_cutoff, force_threshold); + vstorage_.ComputeFilesMarkedForForcedBlobGC( + age_cutoff, force_threshold, /*enable_blob_garbage_collection=*/true); ASSERT_TRUE(vstorage_.FilesMarkedForForcedBlobGC().empty()); } @@ -848,7 +856,8 @@ TEST_F(VersionStorageInfoTest, ForcedBlobGCMultipleBatches) { { constexpr double age_cutoff = 0.5; constexpr double force_threshold = 0.5; - vstorage_.ComputeFilesMarkedForForcedBlobGC(age_cutoff, force_threshold); + vstorage_.ComputeFilesMarkedForForcedBlobGC( + age_cutoff, force_threshold, /*enable_blob_garbage_collection=*/true); auto ssts_to_be_compacted = vstorage_.FilesMarkedForForcedBlobGC(); ASSERT_EQ(ssts_to_be_compacted.size(), 2); @@ -877,7 +886,8 @@ TEST_F(VersionStorageInfoTest, ForcedBlobGCMultipleBatches) { { constexpr double age_cutoff = 0.75; constexpr double force_threshold = 0.6; - vstorage_.ComputeFilesMarkedForForcedBlobGC(age_cutoff, force_threshold); + vstorage_.ComputeFilesMarkedForForcedBlobGC( + age_cutoff, force_threshold, /*enable_blob_garbage_collection=*/true); ASSERT_TRUE(vstorage_.FilesMarkedForForcedBlobGC().empty()); } @@ -888,7 +898,8 @@ TEST_F(VersionStorageInfoTest, ForcedBlobGCMultipleBatches) { { constexpr double age_cutoff = 0.75; constexpr double force_threshold = 0.5; - vstorage_.ComputeFilesMarkedForForcedBlobGC(age_cutoff, force_threshold); + vstorage_.ComputeFilesMarkedForForcedBlobGC( + age_cutoff, force_threshold, /*enable_blob_garbage_collection=*/true); auto ssts_to_be_compacted = vstorage_.FilesMarkedForForcedBlobGC(); ASSERT_EQ(ssts_to_be_compacted.size(), 2);