Test refactoring for Backups+Temperatures (#9655)

Summary:
In preparation for more support for file Temperatures in BackupEngine,
this change does some test refactoring:
* Move DBTest2::BackupFileTemperature test to
BackupEngineTest::FileTemperatures, with some updates to make it work
in the new home. This test will soon be expanded for deeper backup work.
* Move FileTemperatureTestFS from db_test2.cc to db_test_util.h, to
support sharing because of above moved test, but split off the "no link"
part to the test needing it.
* Use custom FileSystems in backupable_db_test rather than custom Envs,
because going through Env file interfaces doesn't support temperatures.
* Fix RemapFileSystem to map DirFsyncOptions::renamed_new_name
parameter to FsyncWithDirOptions, which was required because this
limitation caused a crash only after moving to higher fidelity of
FileSystem interface (vs. LegacyDirectoryWrapper throwing away some
parameter details)
* `backupable_options_` -> `engine_options_` as part of the ongoing
work to get rid of the obsolete "backupable" naming.

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

Test Plan: test code updates only

Reviewed By: jay-zhuang

Differential Revision: D34622183

Pulled By: pdillinger

fbshipit-source-id: f24b7a596a89b9e089e960f4e5d772575513e93f

db/db_test2.cc

@@ -21,7 +21,6 @@
 #include "rocksdb/persistent_cache.h"
 #include "rocksdb/trace_record.h"
 #include "rocksdb/trace_record_result.h"
-#include "rocksdb/utilities/backup_engine.h"
 #include "rocksdb/utilities/replayer.h"
 #include "rocksdb/wal_filter.h"
 #include "test_util/testutil.h"
@@ -6909,126 +6908,18 @@ TEST_F(DBTest2, LastLevelStatistics) {
             options.statistics->getTickerCount(WARM_FILE_READ_COUNT));
 }
 
-class FileTemperatureTestFS : public FileSystemWrapper {
+TEST_F(DBTest2, CheckpointFileTemperature) {
- public:
+  class NoLinkTestFS : public FileTemperatureTestFS {
-  explicit FileTemperatureTestFS(SpecialEnv* env)
+    using FileTemperatureTestFS::FileTemperatureTestFS;
-      : FileSystemWrapper(env->GetFileSystem()) {}
-
-  static const char* kClassName() { return "TestFileSystem"; }
-  const char* Name() const override { return kClassName(); }
-
-  IOStatus NewSequentialFile(const std::string& fname, const FileOptions& opts,
-                             std::unique_ptr<FSSequentialFile>* result,
-                             IODebugContext* dbg) override {
-    auto filename = GetFileName(fname);
-    uint64_t number;
-    FileType type;
-    auto r = ParseFileName(filename, &number, &type);
-    assert(r);
-    if (type == kTableFile) {
-      auto emplaced =
-          requested_sst_file_temperatures_.emplace(number, opts.temperature);
-      assert(emplaced.second);  // assume no duplication
-    }
-    return target()->NewSequentialFile(fname, opts, result, dbg);
-  }
-
-  IOStatus LinkFile(const std::string& s, const std::string& t,
-                    const IOOptions& options, IODebugContext* dbg) override {
-    auto filename = GetFileName(s);
-    uint64_t number;
-    FileType type;
-    auto r = ParseFileName(filename, &number, &type);
-    assert(r);
-    // return not supported to force checkpoint copy the file instead of just
-    // link
-    if (type == kTableFile) {
-      return IOStatus::NotSupported();
-    }
-    return target()->LinkFile(s, t, options, dbg);
-  }
-
-  const std::map<uint64_t, Temperature>& RequestedSstFileTemperatures() {
-    return requested_sst_file_temperatures_;
-  }
-
-  void ClearRequestedFileTemperatures() {
-    requested_sst_file_temperatures_.clear();
-  }
-
- private:
-  std::map<uint64_t, Temperature> requested_sst_file_temperatures_;
-
-  std::string GetFileName(const std::string& fname) {
-    auto filename = fname.substr(fname.find_last_of(kFilePathSeparator) + 1);
-    // workaround only for Windows that the file path could contain both Windows
-    // FilePathSeparator and '/'
-    filename = filename.substr(filename.find_last_of('/') + 1);
-    return filename;
-  }
-};
-
-TEST_F(DBTest2, BackupFileTemperature) {
-  std::shared_ptr<FileTemperatureTestFS> test_fs =
-      std::make_shared<FileTemperatureTestFS>(env_);
-  std::unique_ptr<Env> backup_env(new CompositeEnvWrapper(env_, test_fs));
-  Options options = CurrentOptions();
-  options.bottommost_temperature = Temperature::kWarm;
-  options.level0_file_num_compaction_trigger = 2;
-  Reopen(options);
-
-  // generate a bottommost file and a non-bottommost file
-  ASSERT_OK(Put("foo", "bar"));
-  ASSERT_OK(Put("bar", "bar"));
-  ASSERT_OK(Flush());
-  ASSERT_OK(Put("foo", "bar"));
-  ASSERT_OK(Put("bar", "bar"));
-  ASSERT_OK(Flush());
-  ASSERT_OK(dbfull()->TEST_WaitForCompact());
-  ASSERT_OK(Put("foo", "bar"));
-  ASSERT_OK(Put("bar", "bar"));
-  ASSERT_OK(Flush());
-  auto size = GetSstSizeHelper(Temperature::kWarm);
-  ASSERT_GT(size, 0);
-
-  std::map<uint64_t, Temperature> temperatures;
-  std::vector<LiveFileStorageInfo> infos;
-  ASSERT_OK(
-      dbfull()->GetLiveFilesStorageInfo(LiveFilesStorageInfoOptions(), &infos));
-  for (auto info : infos) {
-    temperatures.emplace(info.file_number, info.temperature);
-  }
-
-  std::unique_ptr<BackupEngine> backup_engine;
-  {
-    BackupEngine* backup_engine_raw_ptr;
-    auto backup_options = BackupEngineOptions(
-        dbname_ + kFilePathSeparator + "tempbk", backup_env.get());
-    ASSERT_OK(BackupEngine::Open(backup_env.get(), backup_options,
-                                 &backup_engine_raw_ptr));
-    backup_engine.reset(backup_engine_raw_ptr);
-  }
-  ASSERT_OK(backup_engine->CreateNewBackup(db_));
 
-  // checking src file src_temperature hints: 2 sst files: 1 sst is kWarm,
+    IOStatus LinkFile(const std::string&, const std::string&, const IOOptions&,
-  // another is kUnknown
+                      IODebugContext*) override {
-  auto file_temperatures = test_fs->RequestedSstFileTemperatures();
+      // return not supported to force checkpoint copy the file instead of just
-  ASSERT_EQ(file_temperatures.size(), 2);
+      // link
-  bool has_only_one_warm_sst = false;
+      return IOStatus::NotSupported();
-  for (const auto& file_temperature : file_temperatures) {
-    ASSERT_EQ(temperatures.at(file_temperature.first), file_temperature.second);
-    if (file_temperature.second == Temperature::kWarm) {
-      ASSERT_FALSE(has_only_one_warm_sst);
-      has_only_one_warm_sst = true;
     }
-  }
+  };
-  ASSERT_TRUE(has_only_one_warm_sst);
+  auto test_fs = std::make_shared<NoLinkTestFS>(env_->GetFileSystem());
-  Close();
-}
-
-TEST_F(DBTest2, CheckpointFileTemperature) {
-  std::shared_ptr<FileTemperatureTestFS> test_fs =
-      std::make_shared<FileTemperatureTestFS>(env_);
   std::unique_ptr<Env> env(new CompositeEnvWrapper(env_, test_fs));
   Options options = CurrentOptions();
   options.bottommost_temperature = Temperature::kWarm;

db/db_test_util.h

@@ -687,6 +687,50 @@ class SpecialEnv : public EnvWrapper {
 };
 
 #ifndef ROCKSDB_LITE
+class FileTemperatureTestFS : public FileSystemWrapper {
+ public:
+  explicit FileTemperatureTestFS(const std::shared_ptr<FileSystem>& fs)
+      : FileSystemWrapper(fs) {}
+
+  static const char* kClassName() { return "FileTemperatureTestFS"; }
+  const char* Name() const override { return kClassName(); }
+
+  IOStatus NewSequentialFile(const std::string& fname, const FileOptions& opts,
+                             std::unique_ptr<FSSequentialFile>* result,
+                             IODebugContext* dbg) override {
+    auto filename = GetFileName(fname);
+    uint64_t number;
+    FileType type;
+    auto r = ParseFileName(filename, &number, &type);
+    assert(r);
+    if (type == kTableFile) {
+      auto emplaced =
+          requested_sst_file_temperatures_.emplace(number, opts.temperature);
+      assert(emplaced.second);  // assume no duplication
+    }
+    return target()->NewSequentialFile(fname, opts, result, dbg);
+  }
+
+  const std::map<uint64_t, Temperature>& RequestedSstFileTemperatures() {
+    return requested_sst_file_temperatures_;
+  }
+
+  void ClearRequestedFileTemperatures() {
+    requested_sst_file_temperatures_.clear();
+  }
+
+ protected:
+  std::map<uint64_t, Temperature> requested_sst_file_temperatures_;
+
+  std::string GetFileName(const std::string& fname) {
+    auto filename = fname.substr(fname.find_last_of(kFilePathSeparator) + 1);
+    // workaround only for Windows that the file path could contain both Windows
+    // FilePathSeparator and '/'
+    filename = filename.substr(filename.find_last_of('/') + 1);
+    return filename;
+  }
+};
+
 class OnFileDeletionListener : public EventListener {
  public:
   OnFileDeletionListener() : matched_count_(0), expected_file_name_("") {}

env/fs_remap.cc

@@ -110,12 +110,45 @@ IOStatus RemapFileSystem::NewDirectory(const std::string& dir,
                                        const IOOptions& options,
                                        std::unique_ptr<FSDirectory>* result,
                                        IODebugContext* dbg) {
+  // A hassle to remap DirFsyncOptions::renamed_new_name
+  class RemapFSDirectory : public FSDirectoryWrapper {
+   public:
+    RemapFSDirectory(RemapFileSystem* fs, std::unique_ptr<FSDirectory>&& t)
+        : FSDirectoryWrapper(std::move(t)), fs_(fs) {}
+    IOStatus FsyncWithDirOptions(
+        const IOOptions& options, IODebugContext* dbg,
+        const DirFsyncOptions& dir_fsync_options) override {
+      if (dir_fsync_options.renamed_new_name.empty()) {
+        return FSDirectoryWrapper::FsyncWithDirOptions(options, dbg,
+                                                       dir_fsync_options);
+      } else {
+        auto status_and_enc_path =
+            fs_->EncodePath(dir_fsync_options.renamed_new_name);
+        if (status_and_enc_path.first.ok()) {
+          DirFsyncOptions mapped_options = dir_fsync_options;
+          mapped_options.renamed_new_name = status_and_enc_path.second;
+          return FSDirectoryWrapper::FsyncWithDirOptions(options, dbg,
+                                                         mapped_options);
+        } else {
+          return status_and_enc_path.first;
+        }
+      }
+    }
+
+   private:
+    RemapFileSystem* const fs_;
+  };
+
   auto status_and_enc_path = EncodePathWithNewBasename(dir);
   if (!status_and_enc_path.first.ok()) {
     return status_and_enc_path.first;
   }
-  return FileSystemWrapper::NewDirectory(status_and_enc_path.second, options,
+  IOStatus ios = FileSystemWrapper::NewDirectory(status_and_enc_path.second,
-                                         result, dbg);
+                                                 options, result, dbg);
+  if (ios.ok()) {
+    *result = std::make_unique<RemapFSDirectory>(this, std::move(*result));
+  }
+  return ios;
 }
 
 IOStatus RemapFileSystem::FileExists(const std::string& fname,
@@ -293,7 +326,7 @@ IOStatus RemapFileSystem::GetAbsolutePath(const std::string& db_path,
                                           const IOOptions& options,
                                           std::string* output_path,
                                           IODebugContext* dbg) {
-  auto status_and_enc_path = EncodePath(db_path);
+  auto status_and_enc_path = EncodePathWithNewBasename(db_path);
   if (!status_and_enc_path.first.ok()) {
     return status_and_enc_path.first;
   }