From b092977643e5a37945afc67455a2ebd9e20ac236 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Wed, 21 Feb 2018 17:33:14 -0800 Subject: [PATCH] BackupEngine gluster-friendly file naming convention Summary: Use the rsync tempfile naming convention in our `BackupEngine`. The temp file follows the format, `..`, which is later renamed to ``. We fix `tmp` as the `` as we don't need to use random bytes for now. The benefit is gluster treats this tempfile naming convention specially and applies hashing only to ``, so the file won't need to be linked or moved when it's renamed. Our gluster team suggested this will make things operationally easier. Closes https://github.com/facebook/rocksdb/pull/3463 Differential Revision: D6893333 Pulled By: ajkr fbshipit-source-id: fd7622978f4b2487fce33cde40dd3124f16bcaa8 --- utilities/backupable/backupable_db.cc | 47 +++++++++++++--------- utilities/backupable/backupable_db_test.cc | 10 ++--- 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/utilities/backupable/backupable_db.cc b/utilities/backupable/backupable_db.cc index e484eea363..bccb7efd22 100644 --- a/utilities/backupable/backupable_db.cc +++ b/utilities/backupable/backupable_db.cc @@ -146,11 +146,17 @@ class BackupEngineImpl : public BackupEngine { class BackupMeta { public: - BackupMeta(const std::string& meta_filename, + BackupMeta( + const std::string& meta_filename, const std::string& meta_tmp_filename, std::unordered_map>* file_infos, Env* env) - : timestamp_(0), sequence_number_(0), size_(0), - meta_filename_(meta_filename), file_infos_(file_infos), env_(env) {} + : timestamp_(0), + sequence_number_(0), + size_(0), + meta_filename_(meta_filename), + meta_tmp_filename_(meta_tmp_filename), + file_infos_(file_infos), + env_(env) {} BackupMeta(const BackupMeta&) = delete; BackupMeta& operator=(const BackupMeta&) = delete; @@ -228,6 +234,7 @@ class BackupEngineImpl : public BackupEngine { uint64_t size_; std::string app_metadata_; std::string const meta_filename_; + std::string const meta_tmp_filename_; // files with relative paths (without "/" prefix!!) std::vector> files_; std::unordered_map>* file_infos_; @@ -257,12 +264,14 @@ class BackupEngineImpl : public BackupEngine { inline std::string GetSharedFileRel(const std::string& file = "", bool tmp = false) const { assert(file.size() == 0 || file[0] != '/'); - return "shared/" + file + (tmp ? ".tmp" : ""); + return std::string("shared/") + (tmp ? "." : "") + file + + (tmp ? ".tmp" : ""); } inline std::string GetSharedFileWithChecksumRel(const std::string& file = "", bool tmp = false) const { assert(file.size() == 0 || file[0] != '/'); - return GetSharedChecksumDirRel() + "/" + file + (tmp ? ".tmp" : ""); + return GetSharedChecksumDirRel() + "/" + (tmp ? "." : "") + file + + (tmp ? ".tmp" : ""); } inline std::string GetSharedFileWithChecksum(const std::string& file, const uint32_t checksum_value, @@ -283,8 +292,9 @@ class BackupEngineImpl : public BackupEngine { inline std::string GetBackupMetaDir() const { return GetAbsolutePath("meta"); } - inline std::string GetBackupMetaFile(BackupID backup_id) const { - return GetBackupMetaDir() + "/" + rocksdb::ToString(backup_id); + inline std::string GetBackupMetaFile(BackupID backup_id, bool tmp) const { + return GetBackupMetaDir() + "/" + (tmp ? "." : "") + + rocksdb::ToString(backup_id) + (tmp ? ".tmp" : ""); } // If size_limit == 0, there is no size limit, copy everything. @@ -605,10 +615,11 @@ Status BackupEngineImpl::Initialize() { continue; } assert(backups_.find(backup_id) == backups_.end()); - backups_.insert( - std::make_pair(backup_id, unique_ptr(new BackupMeta( - GetBackupMetaFile(backup_id), - &backuped_file_infos_, backup_env_)))); + backups_.insert(std::make_pair( + backup_id, unique_ptr(new BackupMeta( + GetBackupMetaFile(backup_id, false /* tmp */), + GetBackupMetaFile(backup_id, true /* tmp */), + &backuped_file_infos_, backup_env_)))); } latest_backup_id_ = 0; @@ -736,10 +747,11 @@ Status BackupEngineImpl::CreateNewBackupWithMetadata( BackupID new_backup_id = latest_backup_id_ + 1; assert(backups_.find(new_backup_id) == backups_.end()); - auto ret = backups_.insert( - std::make_pair(new_backup_id, unique_ptr(new BackupMeta( - GetBackupMetaFile(new_backup_id), - &backuped_file_infos_, backup_env_)))); + auto ret = backups_.insert(std::make_pair( + new_backup_id, unique_ptr(new BackupMeta( + GetBackupMetaFile(new_backup_id, false /* tmp */), + GetBackupMetaFile(new_backup_id, true /* tmp */), + &backuped_file_infos_, backup_env_)))); assert(ret.second == true); auto& new_backup = ret.first->second; new_backup->RecordTimestamp(); @@ -1708,8 +1720,7 @@ Status BackupEngineImpl::BackupMeta::StoreToFile(bool sync) { EnvOptions env_options; env_options.use_mmap_writes = false; env_options.use_direct_writes = false; - s = env_->NewWritableFile(meta_filename_ + ".tmp", &backup_meta_file, - env_options); + s = env_->NewWritableFile(meta_tmp_filename_, &backup_meta_file, env_options); if (!s.ok()) { return s; } @@ -1751,7 +1762,7 @@ Status BackupEngineImpl::BackupMeta::StoreToFile(bool sync) { s = backup_meta_file->Close(); } if (s.ok()) { - s = env_->RenameFile(meta_filename_ + ".tmp", meta_filename_); + s = env_->RenameFile(meta_tmp_filename_, meta_filename_); } return s; } diff --git a/utilities/backupable/backupable_db_test.cc b/utilities/backupable/backupable_db_test.cc index 7a30e4ec3f..b31d273d19 100644 --- a/utilities/backupable/backupable_db_test.cc +++ b/utilities/backupable/backupable_db_test.cc @@ -810,9 +810,9 @@ TEST_F(BackupableDBTest, NoDoubleCopy) { test_db_env_->SetFilenamesForMockedAttrs(dummy_db_->live_files_); ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), false)); std::vector should_have_written = { - "/shared/00010.sst.tmp", "/shared/00011.sst.tmp", + "/shared/.00010.sst.tmp", "/shared/.00011.sst.tmp", "/private/1.tmp/CURRENT", "/private/1.tmp/MANIFEST-01", - "/private/1.tmp/00011.log", "/meta/1.tmp"}; + "/private/1.tmp/00011.log", "/meta/.1.tmp"}; AppendPath(backupdir_, should_have_written); test_backup_env_->AssertWrittenFiles(should_have_written); @@ -828,9 +828,9 @@ TEST_F(BackupableDBTest, NoDoubleCopy) { ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), false)); // should not open 00010.sst - it's already there - should_have_written = {"/shared/00015.sst.tmp", "/private/2.tmp/CURRENT", + should_have_written = {"/shared/.00015.sst.tmp", "/private/2.tmp/CURRENT", "/private/2.tmp/MANIFEST-01", - "/private/2.tmp/00011.log", "/meta/2.tmp"}; + "/private/2.tmp/00011.log", "/meta/.2.tmp"}; AppendPath(backupdir_, should_have_written); test_backup_env_->AssertWrittenFiles(should_have_written); @@ -1169,7 +1169,7 @@ TEST_F(BackupableDBTest, DeleteTmpFiles) { } else { shared_tmp += "/shared"; } - shared_tmp += "/00006.sst.tmp"; + shared_tmp += "/.00006.sst.tmp"; std::string private_tmp_dir = backupdir_ + "/private/10.tmp"; std::string private_tmp_file = private_tmp_dir + "/00003.sst"; file_manager_->WriteToFile(shared_tmp, "tmp");