Replace GC cutoff threshold with a constant (facebook#14272)

Summary:
Pull Request resolved: facebook#14272

All production deployments have the hardcoded (non-configurable) default for`garbage_collection_cutoff = 0.25`. This change removes the configurable option and replaces it with a fixed constant `kGarbageCollectionCutoff = 0.25`, simplifying the configuration surface.

Changes:
- Remove `garbage_collection_cutoff` from `BlobDBOptions`
- Add `kGarbageCollectionCutoff` constant (0.25) in blob_db_impl.cc
- Remove `--blob_db_gc_cutoff` flag from db_bench tool and db_stress
- Update tests to work with the fixed cutoff value

Reviewed By: xingbowang

Differential Revision: D91088998

fbshipit-source-id: 820fc7f1ad4c3fe8a15f22a92cd53fb96c56c6e1

Author: mszeszko-meta

db_stress_tool/db_stress_common.h

@@ -300,7 +300,6 @@ DECLARE_bool(use_blob_db);
 DECLARE_uint64(blob_db_bytes_per_sync);
 DECLARE_uint64(blob_db_file_size);
 DECLARE_bool(blob_db_enable_gc);
-DECLARE_double(blob_db_gc_cutoff);
 
 // Options for integrated BlobDB
 DECLARE_bool(allow_setting_blob_options_dynamically);

db_stress_tool/db_stress_gflags.cc

@@ -439,11 +439,6 @@ DEFINE_bool(
     ROCKSDB_NAMESPACE::blob_db::BlobDBOptions().enable_garbage_collection,
     "[Stacked BlobDB] Enable BlobDB garbage collection.");
 
-DEFINE_double(
-    blob_db_gc_cutoff,
-    ROCKSDB_NAMESPACE::blob_db::BlobDBOptions().garbage_collection_cutoff,
-    "[Stacked BlobDB] Cutoff ratio for BlobDB garbage collection.");
-
 // Options for integrated BlobDB
 DEFINE_bool(allow_setting_blob_options_dynamically, false,
             "[Integrated BlobDB] Allow setting blob options dynamically.");

db_stress_tool/db_stress_test_base.cc

@@ -3847,7 +3847,6 @@ void StressTest::Open(SharedState* shared, bool reopen) {
           blob_db_options.bytes_per_sync = FLAGS_blob_db_bytes_per_sync;
           blob_db_options.blob_file_size = FLAGS_blob_db_file_size;
           blob_db_options.enable_garbage_collection = FLAGS_blob_db_enable_gc;
-          blob_db_options.garbage_collection_cutoff = FLAGS_blob_db_gc_cutoff;
 
           blob_db::BlobDB* blob_db = nullptr;
           s = blob_db::BlobDB::Open(options_, blob_db_options, FLAGS_db,

tools/db_bench_tool.cc

@@ -1050,11 +1050,6 @@ DEFINE_bool(
     ROCKSDB_NAMESPACE::blob_db::BlobDBOptions().enable_garbage_collection,
     "[Stacked BlobDB] Enable BlobDB garbage collection.");
 
-DEFINE_double(
-    blob_db_gc_cutoff,
-    ROCKSDB_NAMESPACE::blob_db::BlobDBOptions().garbage_collection_cutoff,
-    "[Stacked BlobDB] Cutoff ratio for BlobDB garbage collection.");
-
 DEFINE_uint64(blob_db_max_db_size,
               ROCKSDB_NAMESPACE::blob_db::BlobDBOptions().max_db_size,
               "[Stacked BlobDB] Max size limit of the directory where blob "
@@ -5181,7 +5176,6 @@ class Benchmark {
       // Stacked BlobDB
       blob_db::BlobDBOptions blob_db_options;
       blob_db_options.enable_garbage_collection = FLAGS_blob_db_enable_gc;
-      blob_db_options.garbage_collection_cutoff = FLAGS_blob_db_gc_cutoff;
       blob_db_options.max_db_size = FLAGS_blob_db_max_db_size;
       blob_db_options.ttl_range_secs = FLAGS_blob_db_ttl_range_secs;
       blob_db_options.bytes_per_sync = FLAGS_blob_db_bytes_per_sync;

utilities/blob_db/blob_db.cc

@@ -89,9 +89,6 @@ void BlobDBOptions::Dump(Logger* log) const {
   ROCKS_LOG_HEADER(
       log, "                 BlobDBOptions.enable_garbage_collection: %d",
       enable_garbage_collection);
-  ROCKS_LOG_HEADER(
-      log, "                 BlobDBOptions.garbage_collection_cutoff: %f",
-      garbage_collection_cutoff);
   ROCKS_LOG_HEADER(
       log, "                  BlobDBOptions.disable_background_tasks: %d",
       disable_background_tasks);

utilities/blob_db/blob_db.h

@@ -61,11 +61,6 @@ struct BlobDBOptions {
   // by rewriting the remaining live blobs to new files.
   bool enable_garbage_collection = false;
 
-  // The cutoff in terms of blob file age for garbage collection. Blobs in
-  // the oldest N non-TTL blob files will be rewritten when encountered during
-  // compaction, where N = garbage_collection_cutoff * number_of_non_TTL_files.
-  double garbage_collection_cutoff = 0.25;
-
   // Disable all background job. Used for test only.
   bool disable_background_tasks = false;
 

utilities/blob_db/blob_db_impl.cc

@@ -43,6 +43,11 @@
 
 namespace ROCKSDB_NAMESPACE::blob_db {
 
+// The cutoff in terms of blob file age for garbage collection. Blobs in the
+// oldest N non-TTL blob files will be rewritten when encountered during
+// compaction, where N = kGarbageCollectionCutoff * number_of_non_TTL_files.
+constexpr double kGarbageCollectionCutoff = 0.25;
+
 bool BlobFileComparator::operator()(
     const std::shared_ptr<BlobFile>& lhs,
     const std::shared_ptr<BlobFile>& rhs) const {
@@ -138,12 +143,6 @@ Status BlobDBImpl::Open(std::vector<ColumnFamilyHandle*>* handles) {
     return Status::NotSupported("No blob directory in options");
   }
 
-  if (bdb_options_.garbage_collection_cutoff < 0.0 ||
-      bdb_options_.garbage_collection_cutoff > 1.0) {
-    return Status::InvalidArgument(
-        "Garbage collection cutoff must be in the interval [0.0, 1.0]");
-  }
-
   // Temporarily disable compactions in the base DB during open; save the user
   // defined value beforehand so we can restore it once BlobDB is initialized.
   // Note: this is only needed if garbage collection is enabled.
@@ -1185,8 +1184,8 @@ void BlobDBImpl::GetCompactionContext(BlobCompactionContext* context,
 
   if (!live_imm_non_ttl_blob_files_.empty()) {
     auto it = live_imm_non_ttl_blob_files_.begin();
-    std::advance(it, bdb_options_.garbage_collection_cutoff *
-                         live_imm_non_ttl_blob_files_.size());
+    std::advance(
+        it, kGarbageCollectionCutoff * live_imm_non_ttl_blob_files_.size());
     context_gc->cutoff_file_number = it != live_imm_non_ttl_blob_files_.end()
                                          ? it->first
                                          : std::numeric_limits<uint64_t>::max();

utilities/blob_db/blob_db_test.cc

@@ -509,7 +509,6 @@ TEST_F(BlobDBTest, SstFileManager) {
 
   BlobDBOptions bdb_options;
   bdb_options.enable_garbage_collection = true;
-  bdb_options.garbage_collection_cutoff = 1.0;
   Options db_options;
 
   int files_scheduled_to_delete = 0;
@@ -527,20 +526,31 @@ TEST_F(BlobDBTest, SstFileManager) {
 
   Open(bdb_options, db_options);
 
-  // Create one obselete file and clean it.
+  // Create 4 blob files. With GC cutoff of 0.25, the oldest file (file 1)
+  // will be in the GC zone: floor(0.25 * 4) = 1.
   ASSERT_OK(blob_db_->Put(WriteOptions(), "foo", "bar"));
   auto blob_files = blob_db_impl()->TEST_GetBlobFiles();
   ASSERT_EQ(1, blob_files.size());
   std::shared_ptr<BlobFile> bfile = blob_files[0];
   ASSERT_OK(blob_db_impl()->TEST_CloseBlobFile(bfile));
+
+  // Create 3 more blob files (files 2-4, outside GC zone).
+  for (int i = 1; i < 4; i++) {
+    ASSERT_OK(blob_db_->Put(WriteOptions(), "key" + std::to_string(i), "val"));
+    blob_files = blob_db_impl()->TEST_GetBlobFiles();
+    ASSERT_EQ(static_cast<size_t>(i + 1), blob_files.size());
+    ASSERT_OK(blob_db_impl()->TEST_CloseBlobFile(blob_files[i]));
+  }
+
   ASSERT_OK(blob_db_->CompactRange(CompactRangeOptions(), nullptr, nullptr));
   blob_db_impl()->TEST_DeleteObsoleteFiles();
 
   // Even if SSTFileManager is not set, DB is creating a dummy one.
   ASSERT_EQ(1, files_scheduled_to_delete);
   Destroy();
   // Make sure that DestroyBlobDB() also goes through delete scheduler.
-  ASSERT_EQ(2, files_scheduled_to_delete);
+  // Remaining files: 3 original (files 2-4) + 1 GC output file = 4 files.
+  ASSERT_EQ(5, files_scheduled_to_delete);
   SyncPoint::GetInstance()->DisableProcessing();
   sfm->WaitForEmptyTrash();
 }
@@ -606,20 +616,27 @@ TEST_F(BlobDBTest, SstFileManagerRestart) {
 TEST_F(BlobDBTest, SnapshotAndGarbageCollection) {
   BlobDBOptions bdb_options;
   bdb_options.enable_garbage_collection = true;
-  bdb_options.garbage_collection_cutoff = 1.0;
   bdb_options.disable_background_tasks = true;
 
   Options options;
   options.disable_auto_compactions = true;
 
-  // i = when to take snapshot
+  // This test verifies that snapshots protect blob files from deletion during
+  // garbage collection. With fixed GC cutoff of 0.25 and 8 immutable files,
+  // floor(0.25 * 8) = 2 files are in the GC zone (files 1 and 2).
+  //
+  // We run 4 iterations with different snapshot timing:
+  //   i=0: snapshot after key1 (before key2) - protects file 1
+  //   i=1: snapshot after key2 (before key3) - protects files 1 and 2
+  //   i=2: snapshot after key9 (after all keys) - no protection needed
+  //   i=3: snapshot after Delete(key2) - no protection needed
   for (int i = 0; i < 4; i++) {
     Destroy();
     Open(bdb_options, options);
 
     const Snapshot* snapshot = nullptr;
 
-    // First file
+    // Create first blob file (will be in GC zone).
     ASSERT_OK(Put("key1", "value"));
     if (i == 0) {
       snapshot = blob_db_->GetSnapshot();
@@ -629,47 +646,75 @@ TEST_F(BlobDBTest, SnapshotAndGarbageCollection) {
     ASSERT_EQ(1, blob_files.size());
     ASSERT_OK(blob_db_impl()->TEST_CloseBlobFile(blob_files[0]));
 
-    // Second file
+    // Create second blob file (will be in GC zone). We track this file
+    // to verify it becomes obsolete after GC relocates its blob.
     ASSERT_OK(Put("key2", "value"));
     if (i == 1) {
       snapshot = blob_db_->GetSnapshot();
     }
 
     blob_files = blob_db_impl()->TEST_GetBlobFiles();
     ASSERT_EQ(2, blob_files.size());
-    auto bfile = blob_files[1];
-    ASSERT_FALSE(bfile->Immutable());
-    ASSERT_OK(blob_db_impl()->TEST_CloseBlobFile(bfile));
+    auto gc_target_file = blob_files[1];
+    ASSERT_FALSE(gc_target_file->Immutable());
+    ASSERT_OK(blob_db_impl()->TEST_CloseBlobFile(gc_target_file));
+
+    // Create files 3-8, all closed (these are outside GC zone).
+    for (int j = 3; j <= 8; j++) {
+      ASSERT_OK(Put("key" + std::to_string(j), "value"));
+      blob_files = blob_db_impl()->TEST_GetBlobFiles();
+      ASSERT_EQ(static_cast<size_t>(j), blob_files.size());
+      ASSERT_OK(blob_db_impl()->TEST_CloseBlobFile(blob_files[j - 1]));
+    }
 
-    // Third file
-    ASSERT_OK(Put("key3", "value"));
+    // Create file 9 but leave it open (mutable). Only immutable files are
+    // counted for GC cutoff calculation.
+    ASSERT_OK(Put("key9", "value"));
     if (i == 2) {
       snapshot = blob_db_->GetSnapshot();
     }
 
+    // Verify we have 9 total files (8 immutable + 1 mutable).
+    blob_files = blob_db_impl()->TEST_GetBlobFiles();
+    ASSERT_EQ(9, blob_files.size());
+
+    // Trigger GC via compaction. Blobs in files 1 and 2 will be relocated
+    // to a new GC output file.
     ASSERT_OK(blob_db_->CompactRange(CompactRangeOptions(), nullptr, nullptr));
-    ASSERT_TRUE(bfile->Obsolete());
+
+    // Verify gc_target_file (file 2) is now obsolete.
+    ASSERT_TRUE(gc_target_file->Obsolete());
+    // Verify the obsolete sequence matches the latest sequence number.
     ASSERT_EQ(blob_db_->GetLatestSequenceNumber(),
-              bfile->GetObsoleteSequence());
+              gc_target_file->GetObsoleteSequence());
 
     Delete("key2");
     if (i == 3) {
       snapshot = blob_db_->GetSnapshot();
     }
 
-    ASSERT_EQ(4, blob_db_impl()->TEST_GetBlobFiles().size());
+    // Verify we now have 10 files (9 original + 1 GC output file).
+    // Files 1 and 2 are obsolete but not yet deleted.
+    ASSERT_EQ(10, blob_db_impl()->TEST_GetBlobFiles().size());
     blob_db_impl()->TEST_DeleteObsoleteFiles();
 
     if (i >= 2) {
-      // The snapshot shouldn't see data in bfile
-      ASSERT_EQ(2, blob_db_impl()->TEST_GetBlobFiles().size());
+      // Snapshot was taken after all keys were written, so it sees the
+      // post-compaction state where blob indexes point to the GC output file.
+      // Obsolete files 1 and 2 can be deleted immediately.
+      // Verify 8 files remain (10 - 2 obsolete files deleted).
+      ASSERT_EQ(8, blob_db_impl()->TEST_GetBlobFiles().size());
       blob_db_->ReleaseSnapshot(snapshot);
     } else {
-      // The snapshot will see data in bfile, so the file shouldn't be deleted
-      ASSERT_EQ(4, blob_db_impl()->TEST_GetBlobFiles().size());
+      // Snapshot was taken before compaction completed, so it may still
+      // reference blobs in the obsolete files. Files cannot be deleted.
+      // Verify all 10 files still exist.
+      ASSERT_EQ(10, blob_db_impl()->TEST_GetBlobFiles().size());
       blob_db_->ReleaseSnapshot(snapshot);
       blob_db_impl()->TEST_DeleteObsoleteFiles();
-      ASSERT_EQ(2, blob_db_impl()->TEST_GetBlobFiles().size());
+      // After releasing the snapshot, obsolete files can be deleted.
+      // Verify 8 files remain.
+      ASSERT_EQ(8, blob_db_impl()->TEST_GetBlobFiles().size());
     }
   }
 }
@@ -1154,7 +1199,6 @@ TEST_F(BlobDBTest, GarbageCollection) {
   BlobDBOptions bdb_options;
   bdb_options.blob_file_size = kBlobFileSize;
   bdb_options.enable_garbage_collection = true;
-  bdb_options.garbage_collection_cutoff = 0.25;
   bdb_options.disable_background_tasks = true;
 
   Options options;
@@ -1238,8 +1282,9 @@ TEST_F(BlobDBTest, GarbageCollection) {
 
   VerifyBaseDB(blob_value_versions);
 
-  const uint64_t cutoff = static_cast<uint64_t>(
-      bdb_options.garbage_collection_cutoff * kNumBlobFiles);
+  // GC cutoff is fixed at 0.25
+  constexpr double kGCCutoff = 0.25;
+  const uint64_t cutoff = static_cast<uint64_t>(kGCCutoff * kNumBlobFiles);
   for (auto& pair : blob_index_versions) {
     BlobIndexVersion& version = pair.second;
 
@@ -1290,40 +1335,56 @@ TEST_F(BlobDBTest, GarbageCollection) {
 TEST_F(BlobDBTest, GarbageCollectionFailure) {
   BlobDBOptions bdb_options;
   bdb_options.enable_garbage_collection = true;
-  bdb_options.garbage_collection_cutoff = 1.0;
   bdb_options.disable_background_tasks = true;
 
   Options db_options;
   db_options.statistics = CreateDBStatistics();
 
   Open(bdb_options, db_options);
 
-  // Write a couple of valid blobs.
+  // Create 4 blob files. With fixed GC cutoff of 0.25, the oldest file
+  // (floor(0.25 * 4) = 1) will be in the GC zone.
+  // The first file contains valid blobs for "foo" and "dead".
   ASSERT_OK(Put("foo", "bar"));
   ASSERT_OK(Put("dead", "beef"));
 
-  // Write a fake blob reference into the base DB that points to a non-existing
-  // blob file.
+  auto blob_files = blob_db_impl()->TEST_GetBlobFiles();
+  ASSERT_EQ(blob_files.size(), 1);
+  auto first_file = blob_files[0];
+  uint64_t first_file_number = first_file->BlobFileNumber();
+  ASSERT_OK(blob_db_impl()->TEST_CloseBlobFile(first_file));
+
+  // Create 3 more blob files (files 2-4, outside GC zone).
+  for (int i = 1; i < 4; i++) {
+    ASSERT_OK(Put("key" + std::to_string(i), "value"));
+    blob_files = blob_db_impl()->TEST_GetBlobFiles();
+    ASSERT_EQ(static_cast<size_t>(i + 1), blob_files.size());
+    ASSERT_OK(blob_db_impl()->TEST_CloseBlobFile(blob_files[i]));
+  }
+
+  // Write a fake blob index that points to the first file (in GC zone)
+  // but with an invalid offset beyond the file size. This will cause
+  // GC to fail when it tries to read this blob.
   std::string blob_index;
-  BlobIndex::EncodeBlob(&blob_index, /* file_number */ 1000, /* offset */ 1234,
+  BlobIndex::EncodeBlob(&blob_index, first_file_number, /* offset */ 999999,
                         /* size */ 5678, kNoCompression);
 
   WriteBatch batch;
   ASSERT_OK(WriteBatchInternal::PutBlobIndex(
       &batch, blob_db_->DefaultColumnFamily()->GetID(), "key", blob_index));
   ASSERT_OK(blob_db_->GetRootDB()->Write(WriteOptions(), &batch));
 
-  auto blob_files = blob_db_impl()->TEST_GetBlobFiles();
-  ASSERT_EQ(blob_files.size(), 1);
-  auto blob_file = blob_files[0];
-  ASSERT_OK(blob_db_impl()->TEST_CloseBlobFile(blob_file));
-
+  // Verify compaction fails with IO error due to invalid blob offset.
   ASSERT_TRUE(blob_db_->CompactRange(CompactRangeOptions(), nullptr, nullptr)
                   .IsIOError());
 
   const Statistics* const statistics = db_options.statistics.get();
   assert(statistics);
 
+  // Verify GC statistics:
+  // - Relocated 2 keys ("foo" and "dead") with 7 bytes ("bar" + "beef")
+  // - Failed on "key" which has invalid blob offset
+  // - Created 1 new GC output file before failing
   ASSERT_EQ(statistics->getTickerCount(BLOB_DB_GC_NUM_FILES), 0);
   ASSERT_EQ(statistics->getTickerCount(BLOB_DB_GC_NUM_NEW_FILES), 1);
   ASSERT_EQ(statistics->getTickerCount(BLOB_DB_GC_FAILURES), 1);