Remove the need for LATEST_BACKUP in BackupEngine

Summary:
In the first implementation of BackupEngine, LATEST_BACKUP was the commit point. The backup became committed after the write to LATEST_BACKUP completed.

However, we can avoid the need for LATEST_BACKUP. Instead of write to LATEST_BACKUP, the commit point can be the rename from `meta/<backup_id>.tmp` to `meta/<backup_id>`. Once we see that there exists a file `meta/<backup_id>` (without tmp), we can assume that backup is valid.

In this diff, we still write out the file LATEST_BACKUP. We need to do this so that we can maintain backward compatibility. However, the new version doesn't depend on this file anymore. We get the latest backup by `ls`-ing `meta` directory.

This diff depends on D41925

Test Plan: Adjusted backupable_db_test to this new behavior

Reviewers: benj, yhchiang, sdong, AaronFeldman

Reviewed By: sdong

Subscribers: dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D42069

HISTORY.md

@@ -5,6 +5,7 @@
 ### New Features
 * DB::GetProperty() now accept "rocksdb.aggregated-table-properties" and "rocksdb.aggregated-table-properties-at-levelN", in which case it returns aggregated table properties of the target column family, or the aggregated table properties of the specified level N if the "at-level" version is used.
 * Add compression option kZSTDNotFinalCompression for people to experiment ZSTD although its format is not finalized.
+* We removed the need for LATEST_BACKUP file in BackupEngine. We still keep writing it when we create new backups (because of backward compatibility), but we don't read it anymore.
 
 ### Public API Changes
 * Removed class Env::RandomRWFile and Env::NewRandomRWFile().

utilities/backupable/backupable_db.cc

@@ -307,7 +307,6 @@ class BackupEngineImpl : public BackupEngine {
     return GetBackupMetaDir() + "/" + rocksdb::ToString(backup_id);
   }
 
-  Status GetLatestBackupFileContents(uint32_t* latest_backup);
   Status PutLatestBackupFileContents(uint32_t latest_backup);
   // if size_limit == 0, there is no size limit, copy everything
   Status CopyFile(const std::string& src,
@@ -572,6 +571,7 @@ Status BackupEngineImpl::Initialize() {
                                       &backuped_file_infos_, backup_env_)))));
   }
 
+  latest_backup_id_ = 0;
   if (options_.destroy_old_data) {  // Destroy old data
     assert(!read_only_);
     Log(options_.info_log,
@@ -584,8 +584,6 @@ Status BackupEngineImpl::Initialize() {
     if (!s.ok()) {
       return s;
     }
-    // start from beginning
-    latest_backup_id_ = 0;
   } else {  // Load data from storage
     // load the backups if any
     for (auto& backup : backups_) {
@@ -598,52 +596,17 @@ Status BackupEngineImpl::Initialize() {
       } else {
         Log(options_.info_log, "Loading backup %" PRIu32 " OK:\n%s",
             backup.first, backup.second->GetInfoString().c_str());
+        latest_backup_id_ = std::max(latest_backup_id_, backup.first);
       }
     }
 
     for (const auto& corrupt : corrupt_backups_) {
       backups_.erase(backups_.find(corrupt.first));
     }
-
-    Status s = GetLatestBackupFileContents(&latest_backup_id_);
-
-    // If latest backup file is corrupted or non-existent
-    // set latest backup as the biggest backup we have
-    // or 0 if we have no backups
-    if (!s.ok() ||
-        backups_.find(latest_backup_id_) == backups_.end()) {
-      auto itr = backups_.end();
-      latest_backup_id_ = (itr == backups_.begin()) ? 0 : (--itr)->first;
-    }
   }
 
   Log(options_.info_log, "Latest backup is %u", latest_backup_id_);
 
-  // delete any backups that claim to be later than latest
-  std::vector<BackupID> later_ids;
-  for (auto itr = backups_.lower_bound(latest_backup_id_ + 1);
-       itr != backups_.end(); itr++) {
-    Log(options_.info_log,
-        "Found backup claiming to be later than latest: %" PRIu32, itr->first);
-    later_ids.push_back(itr->first);
-  }
-  for (auto id : later_ids) {
-    Status s;
-    if (!read_only_) {
-      s = DeleteBackup(id);
-    } else {
-      auto backup = backups_.find(id);
-      // We just found it couple of lines earlier!
-      assert(backup != backups_.end());
-      s = backup->second->Delete(false);
-      backups_.erase(backup);
-    }
-    if (!s.ok()) {
-      Log(options_.info_log, "Failed deleting backup %" PRIu32 " -- %s", id,
-          s.ToString().c_str());
-    }
-  }
-
   if (!read_only_) {
     auto s = PutLatestBackupFileContents(latest_backup_id_);
     if (!s.ok()) {
@@ -1089,38 +1052,6 @@ Status BackupEngineImpl::RestoreDBFromBackup(
   return s;
 }
 
-// latest backup id is an ASCII representation of latest backup id
-Status BackupEngineImpl::GetLatestBackupFileContents(uint32_t* latest_backup) {
-  Status s;
-  unique_ptr<SequentialFile> file;
-  s = backup_env_->NewSequentialFile(GetLatestBackupFile(),
-                                     &file,
-                                     EnvOptions());
-  if (!s.ok()) {
-    return s;
-  }
-
-  char buf[11];
-  Slice data;
-  unique_ptr<SequentialFileReader> file_reader(
-      new SequentialFileReader(std::move(file)));
-
-  s = file_reader->Read(10, &data, buf);
-  if (!s.ok() || data.size() == 0) {
-    return s.ok() ? Status::Corruption("Latest backup file corrupted") : s;
-  }
-  buf[data.size()] = 0;
-
-  *latest_backup = 0;
-  sscanf(data.data(), "%u", latest_backup);
-
-  s = backup_env_->FileExists(GetBackupMetaFile(*latest_backup));
-  if (s.IsNotFound()) {
-    s = Status::Corruption("Latest backup file corrupted");
-  }
-  return s;
-}
-
 // this operation HAS to be atomic
 // writing 4 bytes to the file is atomic alright, but we should *never*
 // do something like 1. delete file, 2. write new file

utilities/backupable/backupable_db_test.cc

@@ -669,20 +669,28 @@ TEST_F(BackupableDBTest, CorruptionsTest) {
   ASSERT_OK(file_manager_->DeleteFile(backupdir_ + "/LATEST_BACKUP"));
   AssertBackupConsistency(0, 0, keys_iteration * 5);
   // create backup 6, point LATEST_BACKUP to 5
+  // behavior change: this used to delete backup 6. however, now we ignore
+  // LATEST_BACKUP contents so BackupEngine sets latest backup to 6.
   OpenDBAndBackupEngine();
   FillDB(db_.get(), keys_iteration * 5, keys_iteration * 6);
   ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), false));
   CloseDBAndBackupEngine();
   ASSERT_OK(file_manager_->WriteToFile(backupdir_ + "/LATEST_BACKUP", "5"));
-  AssertBackupConsistency(0, 0, keys_iteration * 5, keys_iteration * 6);
-  // assert that all 6 data is gone!
-  ASSERT_EQ(Status::NotFound(),
-            file_manager_->FileExists(backupdir_ + "/meta/6"));
-  ASSERT_EQ(Status::NotFound(),
-            file_manager_->FileExists(backupdir_ + "/private/6"));
+  AssertBackupConsistency(0, 0, keys_iteration * 6);
+  // assert that all 6 data is still here
+  ASSERT_OK(file_manager_->FileExists(backupdir_ + "/meta/6"));
+  ASSERT_OK(file_manager_->FileExists(backupdir_ + "/private/6"));
+  // assert that we wrote 6 to LATEST_BACKUP
+  {
+    std::string latest_backup_contents;
+    ReadFileToString(env_, backupdir_ + "/LATEST_BACKUP",
+                     &latest_backup_contents);
+    ASSERT_EQ(std::atol(latest_backup_contents.c_str()), 6);
+  }
 
   // --------- case 3. corrupted backup meta or missing backuped file ----
   ASSERT_OK(file_manager_->CorruptFile(backupdir_ + "/meta/5", 3));
+  ASSERT_OK(file_manager_->CorruptFile(backupdir_ + "/meta/6", 3));
   // since 5 meta is now corrupted, latest backup should be 4
   AssertBackupConsistency(0, 0, keys_iteration * 4, keys_iteration * 5);
   OpenBackupEngine();
@@ -783,10 +791,11 @@ TEST_F(BackupableDBTest, NoDeleteWithReadOnly) {
   ASSERT_OK(file_manager_->FileExists(backupdir_ + "/meta/5"));
   ASSERT_OK(file_manager_->FileExists(backupdir_ + "/private/5"));
 
-  // even though 5 is here, we should only see 4 backups
+  // Behavior change: We now ignore LATEST_BACKUP contents. This means that
+  // we should have 5 backups, even if LATEST_BACKUP says 4.
   std::vector<BackupInfo> backup_info;
   read_only_backup_engine->GetBackupInfo(&backup_info);
-  ASSERT_EQ(4UL, backup_info.size());
+  ASSERT_EQ(5UL, backup_info.size());
   delete read_only_backup_engine;
 }