Fix GetFileDbIdentities (#7104)

Summary:
Although PR https://github.com/facebook/rocksdb/issues/7032 fixes the construction of the `SstFileDumper` in `GetFileDbIdentities` by setting a proper `Env` of the `Options` passed in the constructor, the file path was not corrected accordingly. This actually disables backup engine to use db session ids in the file names since the `db_session_id` is always empty.

Now it is fixed by setting the correct path in the construction of `SstFileDumper`. Furthermore, to preserve the Direct IO property that backup engine already has, parameter `EnvOptions` is added to `GetFileDbIdentities` and `SstFileDumper`.

The `BackupUsingDirectIO` test is updated accordingly.

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

Test Plan: backupable_db_test and some manual tests.

Reviewed By: ajkr

Differential Revision: D22443245

Pulled By: gg814

fbshipit-source-id: 056a9bb8b82947c5e73d7c3fbb62bfe23af5e562

table/sst_file_dumper.cc

@@ -45,11 +45,12 @@ SstFileDumper::SstFileDumper(const Options& options,
                              const std::string& file_path,
                              size_t readahead_size, bool verify_checksum,
                              bool output_hex, bool decode_blob_index,
-                             bool silent)
+                             const EnvOptions& soptions, bool silent)
     : file_name_(file_path),
       read_num_(0),
       output_hex_(output_hex),
       decode_blob_index_(decode_blob_index),
+      soptions_(soptions),
       silent_(silent),
       options_(options),
       ioptions_(options_),

table/sst_file_dumper.h

@@ -18,6 +18,7 @@ class SstFileDumper {
   explicit SstFileDumper(const Options& options, const std::string& file_name,
                          size_t readahead_size, bool verify_checksum,
                          bool output_hex, bool decode_blob_index,
+                         const EnvOptions& soptions = EnvOptions(),
                          bool silent = false);
 
   Status ReadSequential(bool print_kv, uint64_t read_num, bool has_from,

utilities/backupable/backupable_db.cc

@@ -364,8 +364,9 @@ class BackupEngineImpl : public BackupEngine {
                            uint64_t size_limit, uint32_t* checksum_value);
 
   // Obtain db_id and db_session_id from the table properties of file_path
-  Status GetFileDbIdentities(Env* src_env, const std::string& file_path,
+  Status GetFileDbIdentities(Env* src_env, const EnvOptions& src_env_options,
-                             std::string* db_id, std::string* db_session_id);
+                             const std::string& file_path, std::string* db_id,
+                             std::string* db_session_id);
 
   struct CopyOrCreateResult {
     uint64_t size;
@@ -1546,7 +1547,8 @@ Status BackupEngineImpl::CopyOrCreateFile(
         // SST file
         // Ignore the returned status
         // In the failed cases, db_id and db_session_id will be empty
-        GetFileDbIdentities(src_env, src, db_id, db_session_id);
+        GetFileDbIdentities(src_env, src_env_options, src, db_id,
+                            db_session_id);
       }
     }
   }
@@ -1620,7 +1622,8 @@ Status BackupEngineImpl::AddBackupFileWorkItem(
       // Prepare db_session_id to add to the file name
       // Ignore the returned status
       // In the failed cases, db_id and db_session_id will be empty
-      GetFileDbIdentities(db_env_, src_dir + fname, &db_id, &db_session_id);
+      GetFileDbIdentities(db_env_, src_env_options, src_dir + fname, &db_id,
+                          &db_session_id);
     }
     if (size_bytes == port::kMaxUint64) {
       return Status::NotFound("File missing: " + src_dir + fname);
@@ -1721,7 +1724,8 @@ Status BackupEngineImpl::AddBackupFileWorkItem(
                        "%s checksum checksum calculated, try to obtain DB "
                        "session identity",
                        fname.c_str());
-        GetFileDbIdentities(db_env_, src_dir + fname, &db_id, &db_session_id);
+        GetFileDbIdentities(db_env_, src_env_options, src_dir + fname, &db_id,
+                            &db_session_id);
       }
     }
   }
@@ -1798,20 +1802,20 @@ Status BackupEngineImpl::CalculateChecksum(const std::string& src, Env* src_env,
 }
 
 Status BackupEngineImpl::GetFileDbIdentities(Env* src_env,
+                                             const EnvOptions& src_env_options,
                                              const std::string& file_path,
                                              std::string* db_id,
                                              std::string* db_session_id) {
   assert(db_id != nullptr || db_session_id != nullptr);
-  // // Prepare the full_path of file_path under src_env for SstFileDumper
+
-  std::string full_path;
-  src_env->GetAbsolutePath(file_path, &full_path);
   Options options;
   options.env = src_env;
-  SstFileDumper sst_reader(options, full_path,
+  SstFileDumper sst_reader(options, file_path,
                            2 * 1024 * 1024
                            /* readahead_size */,
                            false /* verify_checksum */, false /* output_hex */,
-                           false /* decode_blob_index */, true /* silent */);
+                           false /* decode_blob_index */, src_env_options,
+                           true /* silent */);
 
   const TableProperties* table_properties = nullptr;
   std::shared_ptr<const TableProperties> tp;
@@ -1826,6 +1830,8 @@ Status BackupEngineImpl::GetFileDbIdentities(Env* src_env,
       table_properties = tp.get();
     }
   } else {
+    ROCKS_LOG_INFO(options_.info_log, "Failed to read %s: %s",
+                   file_path.c_str(), s.ToString().c_str());
     return s;
   }
 
@@ -1836,13 +1842,16 @@ Status BackupEngineImpl::GetFileDbIdentities(Env* src_env,
     if (db_session_id != nullptr) {
       db_session_id->assign(table_properties->db_session_id);
       if (db_session_id->empty()) {
-        return Status::NotFound("DB session identity not found in " +
+        s = Status::NotFound("DB session identity not found in " + file_path);
-                                file_path);
+        ROCKS_LOG_INFO(options_.info_log, "%s", s.ToString().c_str());
+        return s;
       }
     }
     return Status::OK();
   } else {
-    return Status::Corruption("Table properties missing in " + file_path);
+    s = Status::Corruption("Table properties missing in " + file_path);
+    ROCKS_LOG_INFO(options_.info_log, "%s", s.ToString().c_str());
+    return s;
   }
 }
 

utilities/backupable/backupable_db_test.cc

@@ -721,11 +721,8 @@ class BackupableDBTest : public testing::Test {
     if (!s.ok()) {
       return s;
     }
-    for (uint64_t i = 0; i < fsize; ++i) {
+
-      std::string tmp;
+    file_contents[0] = (file_contents[0] + 257) % 256;
-      test::RandomString(&rnd, 1, &tmp);
-      file_contents[rnd.Next() % file_contents.size()] = tmp[0];
-    }
     return WriteStringToFile(test_db_env_.get(), file_contents, fname);
   }
 
@@ -2263,13 +2260,14 @@ TEST_P(BackupableDBTestWithParam, BackupUsingDirectIO) {
 
     // Verify backup engine always opened files with direct I/O
     ASSERT_EQ(0, test_db_env_->num_writers());
-    ASSERT_EQ(0, test_db_env_->num_rand_readers());
+    ASSERT_GT(test_db_env_->num_direct_rand_readers(), 0);
     ASSERT_GT(test_db_env_->num_direct_seq_readers(), 0);
     // Currently the DB doesn't support reading WALs or manifest with direct
     // I/O, so subtract two.
     ASSERT_EQ(test_db_env_->num_seq_readers() - 2,
               test_db_env_->num_direct_seq_readers());
-    ASSERT_EQ(0, test_db_env_->num_rand_readers());
+    ASSERT_EQ(test_db_env_->num_rand_readers(),
+              test_db_env_->num_direct_rand_readers());
   }
   CloseDBAndBackupEngine();