Fix when output level is 0 of universal compaction with trivial move

Summary: Fix for universal compaction with trivial move, when the ouput level is 0. The tests where failing. Fixed by allowing normal compaction when output level is 0. Test Plan: modified test cases run successfully. Reviewers: sdong, yhchiang, IslamAbdelRahman Reviewed By: IslamAbdelRahman Subscribers: anthony, kradhakrishnan, leveldb, dhruba Differential Revision: https://reviews.facebook.net/D42933
mbarbon · Jul 27, 2015 · 1bdfcef · 1bdfcef
1 parent 6a82fba
commit 1bdfcef
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 23 deletions.
diff --git a/db/compaction.cc b/db/compaction.cc
@@ -168,7 +168,8 @@ bool Compaction::IsTrivialMove() const {
 
   // Used in universal compaction, where trivial move can be done if the
   // input files are non overlapping
-  if (cfd_->ioptions()->compaction_options_universal.allow_trivial_move) {
+  if ((cfd_->ioptions()->compaction_options_universal.allow_trivial_move) &&
+      (output_level_ != 0)) {
     return is_trivial_move_;
   }
 

diff --git a/db/compaction_picker.cc b/db/compaction_picker.cc
@@ -1114,7 +1114,7 @@ void UniversalCompactionPicker::SortedRun::DumpSizeInfo(
 
 std::vector<UniversalCompactionPicker::SortedRun>
 UniversalCompactionPicker::CalculateSortedRuns(
-    const VersionStorageInfo& vstorage) {
+    const VersionStorageInfo& vstorage, const ImmutableCFOptions& ioptions) {
   std::vector<UniversalCompactionPicker::SortedRun> ret;
   for (FileMetaData* f : vstorage.LevelFiles(0)) {
     ret.emplace_back(0, f, f->fd.GetFileSize(), f->compensated_file_size,
@@ -1128,10 +1128,18 @@ UniversalCompactionPicker::CalculateSortedRuns(
     for (FileMetaData* f : vstorage.LevelFiles(level)) {
       total_compensated_size += f->compensated_file_size;
       total_size += f->fd.GetFileSize();
-      // Compaction always includes all files for a non-zero level, so for a
-      // non-zero level, all the files should share the same being_compacted
-      // value.
-      assert(is_first || f->being_compacted == being_compacted);
+      if (ioptions.compaction_options_universal.allow_trivial_move == true) {
+        if (f->being_compacted) {
+          being_compacted = f->being_compacted;
+        }
+      } else {
+        // Compaction always includes all files for a non-zero level, so for a
+        // non-zero level, all the files should share the same being_compacted
+        // value.
+        // This assumption is only valid when
+        // ioptions.compaction_options_universal.allow_trivial_move is false
+        assert(is_first || f->being_compacted == being_compacted);
+      }
       if (is_first) {
         being_compacted = f->being_compacted;
         is_first = false;
@@ -1223,7 +1231,8 @@ Compaction* UniversalCompactionPicker::PickCompaction(
     VersionStorageInfo* vstorage, LogBuffer* log_buffer) {
   const int kLevel0 = 0;
   double score = vstorage->CompactionScore(kLevel0);
-  std::vector<SortedRun> sorted_runs = CalculateSortedRuns(*vstorage);
+  std::vector<SortedRun> sorted_runs =
+      CalculateSortedRuns(*vstorage, ioptions_);
 
   if (sorted_runs.size() <
       (unsigned int)mutable_cf_options.level0_file_num_compaction_trigger) {

diff --git a/db/compaction_picker.h b/db/compaction_picker.h
@@ -275,7 +275,7 @@ class UniversalCompactionPicker : public CompactionPicker {
       const std::vector<SortedRun>& sorted_runs, LogBuffer* log_buffer);
 
   static std::vector<SortedRun> CalculateSortedRuns(
-      const VersionStorageInfo& vstorage);
+      const VersionStorageInfo& vstorage, const ImmutableCFOptions& ioptions);
 
   // Pick a path ID to place a newly generated file, with its estimated file
   // size.

diff --git a/db/db_impl.cc b/db/db_impl.cc
@@ -2542,8 +2542,7 @@ Status DBImpl::BackgroundCompaction(bool* madeProgress, JobContext* job_context,
     int32_t moved_files = 0;
     int64_t moved_bytes = 0;
     for (unsigned int l = 0; l < c->num_input_levels(); l++) {
-      if (l == static_cast<unsigned int>(c->output_level()) ||
-          (c->output_level() == 0)) {
+      if (c->level(l) == c->output_level()) {
         continue;
       }
       for (size_t i = 0; i < c->num_input_files(l); i++) {
@@ -2591,7 +2590,9 @@ Status DBImpl::BackgroundCompaction(bool* madeProgress, JobContext* job_context,
     // Clear Instrument
     ThreadStatusUtil::ResetThreadStatus();
   } else {
-    TEST_SYNC_POINT("DBImpl::BackgroundCompaction:NonTrivial");
+    int output_level  __attribute__((unused)) = c->output_level();
+    TEST_SYNC_POINT_CALLBACK("DBImpl::BackgroundCompaction:NonTrivial",
+                             &output_level);
     assert(is_snapshot_supported_ || snapshots_.empty());
     CompactionJob compaction_job(
         job_context->job_id, c.get(), db_options_, env_options_,

diff --git a/db/db_universal_compaction_test.cc b/db/db_universal_compaction_test.cc
@@ -452,8 +452,12 @@ TEST_P(DBTestUniversalCompactionMultiLevels, UniversalCompactionTrivialMove) {
       "DBImpl::BackgroundCompaction:TrivialMove",
       [&](void* arg) { trivial_move++; });
   rocksdb::SyncPoint::GetInstance()->SetCallBack(
-      "DBImpl::BackgroundCompaction:NonTrivial",
-      [&](void* arg) { non_trivial_move++; });
+      "DBImpl::BackgroundCompaction:NonTrivial", [&](void* arg) {
+        non_trivial_move++;
+        ASSERT_TRUE(arg != nullptr);
+        int output_level = *(static_cast<int*>(arg));
+        ASSERT_EQ(output_level, 0);
+      });
   rocksdb::SyncPoint::GetInstance()->EnableProcessing();
 
   Options options;
@@ -462,7 +466,7 @@ TEST_P(DBTestUniversalCompactionMultiLevels, UniversalCompactionTrivialMove) {
   options.num_levels = 3;
   options.write_buffer_size = 100 << 10;  // 100KB
   options.level0_file_num_compaction_trigger = 3;
-  options.max_background_compactions = 1;
+  options.max_background_compactions = 2;
   options.target_file_size_base = 32 * 1024;
   options = CurrentOptions(options);
   DestroyAndReopen(options);
@@ -474,7 +478,7 @@ TEST_P(DBTestUniversalCompactionMultiLevels, UniversalCompactionTrivialMove) {
   ReopenWithColumnFamilies({"default", "pikachu"}, options);
 
   Random rnd(301);
-  int num_keys = 15000;
+  int num_keys = 150000;
   for (int i = 0; i < num_keys; i++) {
     ASSERT_OK(Put(1, Key(i), Key(i)));
   }
@@ -484,7 +488,7 @@ TEST_P(DBTestUniversalCompactionMultiLevels, UniversalCompactionTrivialMove) {
   dbfull()->TEST_WaitForCompact();
 
   ASSERT_GT(trivial_move, 0);
-  ASSERT_EQ(non_trivial_move, 0);
+  ASSERT_GT(non_trivial_move, 0);
 
   rocksdb::SyncPoint::GetInstance()->DisableProcessing();
 }
@@ -789,14 +793,18 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionTrivialMoveTest1) {
       "DBImpl::BackgroundCompaction:TrivialMove",
       [&](void* arg) { trivial_move++; });
   rocksdb::SyncPoint::GetInstance()->SetCallBack(
-      "DBImpl::BackgroundCompaction:NonTrivial",
-      [&](void* arg) { non_trivial_move++; });
+      "DBImpl::BackgroundCompaction:NonTrivial", [&](void* arg) {
+        non_trivial_move++;
+        ASSERT_TRUE(arg != nullptr);
+        int output_level = *(static_cast<int*>(arg));
+        ASSERT_EQ(output_level, 0);
+      });
   rocksdb::SyncPoint::GetInstance()->EnableProcessing();
 
   Options options;
   options.compaction_style = kCompactionStyleUniversal;
   options.compaction_options_universal.allow_trivial_move = true;
-  options.num_levels = 3;
+  options.num_levels = 2;
   options.write_buffer_size = 100 << 10;  // 100KB
   options.level0_file_num_compaction_trigger = 3;
   options.max_background_compactions = 1;
@@ -811,7 +819,7 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionTrivialMoveTest1) {
   ReopenWithColumnFamilies({"default", "pikachu"}, options);
 
   Random rnd(301);
-  int num_keys = 150000;
+  int num_keys = 250000;
   for (int i = 0; i < num_keys; i++) {
     ASSERT_OK(Put(1, Key(i), Key(i)));
   }
@@ -821,7 +829,7 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionTrivialMoveTest1) {
   dbfull()->TEST_WaitForCompact();
 
   ASSERT_GT(trivial_move, 0);
-  ASSERT_EQ(non_trivial_move, 0);
+  ASSERT_GT(non_trivial_move, 0);
 
   rocksdb::SyncPoint::GetInstance()->DisableProcessing();
 }
@@ -835,6 +843,7 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionTrivialMoveTest2) {
   rocksdb::SyncPoint::GetInstance()->SetCallBack(
       "DBImpl::BackgroundCompaction:NonTrivial",
       [&](void* arg) { non_trivial_move++; });
+
   rocksdb::SyncPoint::GetInstance()->EnableProcessing();
 
   Options options;

diff --git a/util/auto_roll_logger_test.cc b/util/auto_roll_logger_test.cc
@@ -28,8 +28,7 @@ class AutoRollLoggerTest : public testing::Test {
     // become confused
     std::string testDir(kTestDir);
     std::replace_if(testDir.begin(), testDir.end(),
-      [](char ch) { return ch == '/'; },
-      '\\');
+                    [](char ch) { return ch == '/'; }, '\\');
     std::string deleteCmd = "if exist " + testDir + " rd /s /q " + testDir;
 #else
     std::string deleteCmd = "rm -rf " + kTestDir;