From 22270dea6fb7fb050de0ad44e9d42fd355e408fb Mon Sep 17 00:00:00 2001
From: Maciej Szeszko <mszeszko@meta.com>
Date: Wed, 5 Feb 2025 11:59:40 -0800
Subject: [PATCH] Fix flaky test post final DB::DeleteFile refactoring (#13349)

Summary:
`DynamicLevelCompressionPerLevel` test started _somewhat occasionally_ failing post refactoring in https://github.com/facebook/rocksdb/pull/13322. In order for `DeleteFilesInRange`-replacement to behave according to our expectations (that is delete exactly that very single file given its' key range), we must first ensure that input `keys` are NOT randomly shuffled, but rather preserved in their natural, sequential order. That change was originally a part of the PR, but got somehow deleted due to human error and since tests passed locally and in CI, spilled unnoticed. We're removing random keys reshuffling (as intended originally) and, in addition, asserting that all such constructed files are 1) non-overlapping and 2) contain full range of keys BEFORE we actually get to test the on table deletion callbacks.

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

Test Plan: Confirmed that key range overlap is an issue by volume testing: `./db_test --gtest_filter=*DynamicLevelCompressionPerLevel --gtest_repeat=1000 --gtest_break_on_failure` (2-3 times is enough). Could not longer repro after the fix.

Reviewed By: jaykorean

Differential Revision: D68857018

Pulled By: mszeszko-meta

fbshipit-source-id: 873b1ba44f32d40192da4265aeeb39702c22a1d0
---
 db/db_test.cc     | 36 ++++++++++++++++++++++++++++++++++--
 db/db_test_util.h |  4 ++++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/db/db_test.cc b/db/db_test.cc
index 6a497cd390d..e141e562afb 100644
--- a/db/db_test.cc
+++ b/db/db_test.cc
@@ -5290,7 +5290,6 @@ TEST_F(DBTest, DynamicLevelCompressionPerLevel) {
   for (int i = 0; i < kNKeys; i++) {
     keys[i] = i;
   }
-  RandomShuffle(std::begin(keys), std::end(keys), 301);
 
   Random rnd(301);
   Options options;
@@ -5346,7 +5345,7 @@ TEST_F(DBTest, DynamicLevelCompressionPerLevel) {
   // above compression settings for each level, there will be some compression.
   ASSERT_OK(options.statistics->Reset());
   ASSERT_EQ(num_block_compressed, 0);
-  for (int i = 21; i < 120; i++) {
+  for (int i = 20; i < 120; i++) {
     ASSERT_OK(Put(Key(keys[i]), CompressibleString(&rnd, 4000)));
     ASSERT_OK(dbfull()->TEST_WaitForBackgroundWork());
   }
@@ -5369,10 +5368,43 @@ TEST_F(DBTest, DynamicLevelCompressionPerLevel) {
   }));
   ColumnFamilyMetaData cf_meta;
   db_->GetColumnFamilyMetaData(&cf_meta);
+
+  // Ensure that L1+ files are non-overlapping and together with L0 encompass
+  // full key range between smallestkey and largestkey from CF file metadata.
+  int largestkey_in_prev_level = -1;
+  int keys_found = 0;
+  for (int level = (int)cf_meta.levels.size() - 1; level >= 0; level--) {
+    int files_in_level = (int)cf_meta.levels[level].files.size();
+    int largestkey_in_prev_file = -1;
+    for (int j = 0; j < files_in_level; j++) {
+      int smallestkey = IdFromKey(cf_meta.levels[level].files[j].smallestkey);
+      int largestkey = IdFromKey(cf_meta.levels[level].files[j].largestkey);
+      int num_entries = (int)cf_meta.levels[level].files[j].num_entries;
+      ASSERT_EQ(num_entries, largestkey - smallestkey + 1);
+      keys_found += num_entries;
+      if (level > 0) {
+        if (j == 0) {
+          ASSERT_GT(smallestkey, largestkey_in_prev_level);
+        }
+        if (j > 0) {
+          ASSERT_GT(smallestkey, largestkey_in_prev_file);
+        }
+        if (j == files_in_level - 1) {
+          largestkey_in_prev_level = largestkey;
+        }
+      }
+      largestkey_in_prev_file = largestkey;
+    }
+  }
+  ASSERT_EQ(keys_found, kNKeys);
+
   for (const auto& file : cf_meta.levels[4].files) {
     listener->SetExpectedFileName(dbname_ + file.name);
     Slice start(file.smallestkey), limit(file.largestkey);
     const RangePtr ranges(&start, &limit);
+    // Given verification from above, we're guaranteed that by deleting all the
+    // files in [<smallestkey>, <largestkey>] range, we're effectively deleting
+    // that very single file and nothing more.
     EXPECT_OK(dbfull()->DeleteFilesInRanges(dbfull()->DefaultColumnFamily(),
                                             &ranges, true /* include_end */));
   }
diff --git a/db/db_test_util.h b/db/db_test_util.h
index 4b5bc48450d..8be174fac68 100644
--- a/db/db_test_util.h
+++ b/db/db_test_util.h
@@ -1106,6 +1106,10 @@ class DBTestBase : public testing::Test {
     return std::string(buf);
   }
 
+  static int IdFromKey(const std::string& key) {
+    return std::stoi(key.substr(3));
+  }
+
   static bool ShouldSkipOptions(int option_config, int skip_mask = kNoSkip);
 
   // Switch to a fresh database with the next option configuration to