Remove hard-coded cf name from a unit test and address comments

facebook · Oct 24, 2019 · e87e0be · e87e0be
1 parent fd69ff7
commit e87e0be
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 46 deletions.
diff --git a/db/db_impl/db_impl_open.cc b/db/db_impl/db_impl_open.cc
@@ -1023,6 +1023,9 @@ Status DBImpl::RecoverLogFiles(const std::vector<uint64_t>& log_numbers,
         continue;
       }
 
+      TEST_SYNC_POINT_CALLBACK(
+          "DBImpl::RecoverLogFiles:BeforeFlushFinalMemtable", /*arg=*/nullptr);
+
       // flush the final memtable (if non-empty)
       if (cfd->mem()->GetFirstSequenceNumber() != 0) {
         // If flush happened in the middle of recovery (e.g. due to memtable

diff --git a/db/db_test2.cc b/db/db_test2.cc
@@ -4116,55 +4116,66 @@ TEST_F(DBTest2, RowCacheSnapshot) {
 // to be corrupted, and triggering a corruption report.
 // We need to fix the bug and enable the test.
 TEST_F(DBTest2, CrashInRecoveryMultipleCF) {
-  Options options = CurrentOptions();
-  options.create_if_missing = true;
-  options.wal_recovery_mode = WALRecoveryMode::kPointInTimeRecovery;
-  CreateAndReopenWithCF({"pikachu"}, options);
-  ASSERT_OK(Put("foo", "bar"));
-  ASSERT_OK(Flush());
-  ASSERT_OK(Put(1, "foo", "bar"));
-  ASSERT_OK(Flush(1));
-  ASSERT_OK(Put("foo", "bar"));
-  ASSERT_OK(Put(1, "foo", "bar"));
-  // The value is large enough to be divided to two blocks.
-  std::string large_value(400, ' ');
-  ASSERT_OK(Put("foo1", large_value));
-  ASSERT_OK(Put("foo2", large_value));
-  Close();
+  const std::vector<std::string> sync_points = {
+      "DBImpl::RecoverLogFiles:BeforeFlushFinalMemtable",
+      "VersionSet::ProcessManifestWrites:BeforeWriteLastVersionEdit:0"};
+  for (const auto& test_sync_point : sync_points) {
+    Options options = CurrentOptions();
+    // First destroy original db to ensure a clean start.
+    DestroyAndReopen(options);
+    options.create_if_missing = true;
+    options.wal_recovery_mode = WALRecoveryMode::kPointInTimeRecovery;
+    CreateAndReopenWithCF({"pikachu"}, options);
+    ASSERT_OK(Put("foo", "bar"));
+    ASSERT_OK(Flush());
+    ASSERT_OK(Put(1, "foo", "bar"));
+    ASSERT_OK(Flush(1));
+    ASSERT_OK(Put("foo", "bar"));
+    ASSERT_OK(Put(1, "foo", "bar"));
+    // The value is large enough to be divided to two blocks.
+    std::string large_value(400, ' ');
+    ASSERT_OK(Put("foo1", large_value));
+    ASSERT_OK(Put("foo2", large_value));
+    Close();
 
-  // Corrupt the log file in the middle, so that it is not corrupted
-  // in the tail.
-  std::vector<std::string> filenames;
-  ASSERT_OK(env_->GetChildren(dbname_, &filenames));
-  for (const auto& f : filenames) {
-    uint64_t number;
-    FileType type;
-    if (ParseFileName(f, &number, &type) && type == FileType::kLogFile) {
-      std::string fname = dbname_ + "/" + f;
-      std::string file_content;
-      ASSERT_OK(ReadFileToString(env_, fname, &file_content));
-      file_content[400] = 'h';
-      file_content[401] = 'a';
-      ASSERT_OK(WriteStringToFile(env_, file_content, fname));
-      break;
+    // Corrupt the log file in the middle, so that it is not corrupted
+    // in the tail.
+    std::vector<std::string> filenames;
+    ASSERT_OK(env_->GetChildren(dbname_, &filenames));
+    for (const auto& f : filenames) {
+      uint64_t number;
+      FileType type;
+      if (ParseFileName(f, &number, &type) && type == FileType::kLogFile) {
+        std::string fname = dbname_ + "/" + f;
+        std::string file_content;
+        ASSERT_OK(ReadFileToString(env_, fname, &file_content));
+        file_content[400] = 'h';
+        file_content[401] = 'a';
+        ASSERT_OK(WriteStringToFile(env_, file_content, fname));
+        break;
+      }
     }
-  }
 
-  // Reopen and freeze the file system after the first manifest write.
-  FaultInjectionTestEnv fit_env(options.env);
-  options.env = &fit_env;
-  rocksdb::SyncPoint::GetInstance()->SetCallBack(
-      "VersionSet::ProcessManifestWrites:BeforeWriteLastVersionEdit:0",
-      [&](void* /*arg*/) { fit_env.SetFilesystemActive(false); });
-  rocksdb::SyncPoint::GetInstance()->EnableProcessing();
-  ASSERT_NOK(TryReopenWithColumnFamilies({"default", "pikachu"}, options));
-  rocksdb::SyncPoint::GetInstance()->DisableProcessing();
-
-  fit_env.SetFilesystemActive(true);
-  // If we continue using failure ingestion Env, it will conplain something
-  // when renaming current file, which is not expected. Need to investigate why.
-  options.env = env_;
-  ASSERT_OK(TryReopenWithColumnFamilies({"default", "pikachu"}, options));
+    // Reopen and freeze the file system after the first manifest write.
+    FaultInjectionTestEnv fit_env(options.env);
+    options.env = &fit_env;
+    rocksdb::SyncPoint::GetInstance()->ClearAllCallBacks();
+    rocksdb::SyncPoint::GetInstance()->SetCallBack(
+        test_sync_point,
+        [&](void* /*arg*/) { fit_env.SetFilesystemActive(false); });
+    rocksdb::SyncPoint::GetInstance()->EnableProcessing();
+    ASSERT_NOK(TryReopenWithColumnFamilies(
+        {kDefaultColumnFamilyName, "pikachu"}, options));
+    rocksdb::SyncPoint::GetInstance()->DisableProcessing();
+
+    fit_env.SetFilesystemActive(true);
+    // If we continue using failure ingestion Env, it will conplain something
+    // when renaming current file, which is not expected. Need to investigate
+    // why.
+    options.env = env_;
+    ASSERT_OK(TryReopenWithColumnFamilies({kDefaultColumnFamilyName, "pikachu"},
+                                          options));
+  }
 }
 }  // namespace rocksdb