From d3ed796855ef4615426cba9ba3188b7c43beb37b Mon Sep 17 00:00:00 2001
From: Levi Tamasi <ltamasi@fb.com>
Date: Fri, 28 Apr 2023 14:07:45 -0700
Subject: [PATCH] Deflake some old BlobDB test cases (#11417)

Summary:
The old `StackableDB` based BlobDB implementation relies on a DB listener to track the total size of the SST files in the database and to trigger FIFO eviction. Some test cases in `BlobDBTest` assume that the listener is notified by the time `DB::Flush` returns, which is not guaranteed (side note: `TEST_WaitForFlushMemTable` would not guarantee this either). The patch fixes these tests by using `SyncPoint`s to make sure the listener is actually called before verifying the FIFO behavior.

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

Test Plan:
```
make -j56 COERCE_CONTEXT_SWITCH=1 blob_db_test
./blob_db_test --gtest_filter=BlobDBTest.FIFOEviction_TriggerOnSSTSizeChange
./blob_db_test --gtest_filter=BlobDBTest.FilterForFIFOEviction
./blob_db_test --gtest_filter=BlobDBTest.FIFOEviction_NoEnoughBlobFilesToEvict
```

Reviewed By: ajkr

Differential Revision: D45407135

Pulled By: ltamasi

fbshipit-source-id: fcd63d76937d2c975f569a6635ce8730772a3d75
---
 utilities/blob_db/blob_db_test.cc | 35 ++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/utilities/blob_db/blob_db_test.cc b/utilities/blob_db/blob_db_test.cc
index 3f25c22a2..015ceb907 100644
--- a/utilities/blob_db/blob_db_test.cc
+++ b/utilities/blob_db/blob_db_test.cc
@@ -1187,6 +1187,12 @@ TEST_F(BlobDBTest, FIFOEviction_NoEnoughBlobFilesToEvict) {
   options.statistics = statistics;
   Open(bdb_options, options);
 
+  SyncPoint::GetInstance()->LoadDependency(
+      {{"DBImpl::NotifyOnFlushCompleted::PostAllOnFlushCompleted",
+        "BlobDBTest.FIFOEviction_NoEnoughBlobFilesToEvict:AfterFlush"}});
+
+  SyncPoint::GetInstance()->EnableProcessing();
+
   ASSERT_EQ(0, blob_db_impl()->TEST_live_sst_size());
   std::string small_value(50, 'v');
   std::map<std::string, std::string> data;
@@ -1196,10 +1202,15 @@ TEST_F(BlobDBTest, FIFOEviction_NoEnoughBlobFilesToEvict) {
     ASSERT_OK(Put("key" + std::to_string(i), small_value, &data));
   }
   ASSERT_OK(blob_db_->Flush(FlushOptions()));
+
   uint64_t live_sst_size = 0;
   ASSERT_TRUE(blob_db_->GetIntProperty(DB::Properties::kTotalSstFilesSize,
                                        &live_sst_size));
   ASSERT_TRUE(live_sst_size > 0);
+
+  TEST_SYNC_POINT(
+      "BlobDBTest.FIFOEviction_NoEnoughBlobFilesToEvict:AfterFlush");
+
   ASSERT_EQ(live_sst_size, blob_db_impl()->TEST_live_sst_size());
 
   bdb_options.max_db_size = live_sst_size + 2000;
@@ -1223,6 +1234,8 @@ TEST_F(BlobDBTest, FIFOEviction_NoEnoughBlobFilesToEvict) {
   ASSERT_EQ(1, statistics->getTickerCount(BLOB_DB_FIFO_NUM_FILES_EVICTED));
   // Verify large_key2 still exists.
   VerifyDB(data);
+
+  SyncPoint::GetInstance()->DisableProcessing();
 }
 
 // Test flush or compaction will trigger FIFO eviction since they update
@@ -1241,6 +1254,12 @@ TEST_F(BlobDBTest, FIFOEviction_TriggerOnSSTSizeChange) {
   options.compression = kNoCompression;
   Open(bdb_options, options);
 
+  SyncPoint::GetInstance()->LoadDependency(
+      {{"DBImpl::NotifyOnFlushCompleted::PostAllOnFlushCompleted",
+        "BlobDBTest.FIFOEviction_TriggerOnSSTSizeChange:AfterFlush"}});
+
+  SyncPoint::GetInstance()->EnableProcessing();
+
   std::string value(800, 'v');
   ASSERT_OK(PutWithTTL("large_key", value, 60));
   ASSERT_EQ(1, blob_db_impl()->TEST_GetBlobFiles().size());
@@ -1254,11 +1273,15 @@ TEST_F(BlobDBTest, FIFOEviction_TriggerOnSSTSizeChange) {
   }
   ASSERT_OK(blob_db_->Flush(FlushOptions()));
 
+  TEST_SYNC_POINT("BlobDBTest.FIFOEviction_TriggerOnSSTSizeChange:AfterFlush");
+
   // Verify large_key is deleted by FIFO eviction.
   blob_db_impl()->TEST_DeleteObsoleteFiles();
   ASSERT_EQ(0, blob_db_impl()->TEST_GetBlobFiles().size());
   ASSERT_EQ(1, statistics->getTickerCount(BLOB_DB_FIFO_NUM_FILES_EVICTED));
   VerifyDB(data);
+
+  SyncPoint::GetInstance()->DisableProcessing();
 }
 
 TEST_F(BlobDBTest, InlineSmallValues) {
@@ -1637,6 +1660,12 @@ TEST_F(BlobDBTest, FilterForFIFOEviction) {
   options.disable_auto_compactions = true;
   Open(bdb_options, options);
 
+  SyncPoint::GetInstance()->LoadDependency(
+      {{"DBImpl::NotifyOnFlushCompleted::PostAllOnFlushCompleted",
+        "BlobDBTest.FilterForFIFOEviction:AfterFlush"}});
+
+  SyncPoint::GetInstance()->EnableProcessing();
+
   std::map<std::string, std::string> data;
   std::map<std::string, std::string> data_after_compact;
   // Insert some small values that will be inlined.
@@ -1651,6 +1680,9 @@ TEST_F(BlobDBTest, FilterForFIFOEviction) {
   }
   uint64_t num_keys_to_evict = data.size() - data_after_compact.size();
   ASSERT_OK(blob_db_->Flush(FlushOptions()));
+
+  TEST_SYNC_POINT("BlobDBTest.FilterForFIFOEviction:AfterFlush");
+
   uint64_t live_sst_size = blob_db_impl()->TEST_live_sst_size();
   ASSERT_GT(live_sst_size, 0);
   VerifyDB(data);
@@ -1702,6 +1734,8 @@ TEST_F(BlobDBTest, FilterForFIFOEviction) {
   data_after_compact["large_key2"] = large_value;
   data_after_compact["large_key3"] = large_value;
   VerifyDB(data_after_compact);
+
+  SyncPoint::GetInstance()->DisableProcessing();
 }
 
 TEST_F(BlobDBTest, GarbageCollection) {
@@ -2394,4 +2428,3 @@ int main(int argc, char **argv) {
   ::testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();
 }
-