From 064294081b945d5f7ded42215b4b8736b5f4f006 Mon Sep 17 00:00:00 2001 From: agiardullo Date: Mon, 20 Jul 2015 17:20:40 -0700 Subject: [PATCH] Improved FileExists API Summary: Add new CheckFileExists method. Considered changing the FileExists api but didn't want to break anyone's builds. Test Plan: unit tests Reviewers: yhchiang, igor, sdong Reviewed By: sdong Subscribers: dhruba, leveldb Differential Revision: https://reviews.facebook.net/D42003 --- HISTORY.md | 1 + db/compact_files_test.cc | 2 +- db/db_impl.cc | 15 ++++- db/db_test.cc | 6 +- db/deletefile_test.cc | 12 ++-- db/fault_injection_test.cc | 8 ++- db/wal_manager.cc | 14 ++-- hdfs/env_hdfs.h | 6 +- include/rocksdb/env.h | 10 ++- include/rocksdb/status.h | 10 +++ port/win/env_win.cc | 5 +- table/cuckoo_table_reader_test.cc | 3 +- util/auto_roll_logger_test.cc | 6 +- util/env_hdfs.cc | 12 ++-- util/env_posix.cc | 28 ++++++-- util/memenv.cc | 8 ++- util/memenv_test.cc | 10 +-- util/mock_env.cc | 8 +-- util/mock_env.h | 2 +- util/mock_env_test.cc | 10 +-- utilities/backupable/backupable_db.cc | 31 +++++++-- utilities/backupable/backupable_db_test.cc | 74 +++++++++++++--------- utilities/checkpoint/checkpoint.cc | 6 +- 23 files changed, 191 insertions(+), 96 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 7127d7327..33721c8f4 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -7,6 +7,7 @@ * Deprecated purge_redundant_kvs_while_flush option. * Removed BackupEngine::NewBackupEngine() and NewReadOnlyBackupEngine() that were deprecated in RocksDB 3.8. Please use BackupEngine::Open() instead. * Deprecated Compaction Filter V2. We are not aware of any existing use-cases. If you use this filter, your compile will break with RocksDB 3.13. Please let us know if you use it and we'll put it back in RocksDB 3.14. +* Env::FileExists now returns a Status instead of a boolean ## 3.12.0 (7/2/2015) ### New Features diff --git a/db/compact_files_test.cc b/db/compact_files_test.cc index c52a3857f..cbd9d7a09 100644 --- a/db/compact_files_test.cc +++ b/db/compact_files_test.cc @@ -91,7 +91,7 @@ TEST_F(CompactFilesTest, ObsoleteFiles) { // verify all compaction input files are deleted for (auto fname : l0_files) { - ASSERT_TRUE(!env_->FileExists(fname)); + ASSERT_EQ(Status::NotFound(), env_->FileExists(fname)); } delete db; } diff --git a/db/db_impl.cc b/db/db_impl.cc index 030f182d1..291493e7e 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -868,7 +868,8 @@ Status DBImpl::Recover( return s; } - if (!env_->FileExists(CurrentFileName(dbname_))) { + s = env_->FileExists(CurrentFileName(dbname_)); + if (s.IsNotFound()) { if (db_options_.create_if_missing) { s = NewDB(); is_new_db = true; @@ -879,18 +880,26 @@ Status DBImpl::Recover( return Status::InvalidArgument( dbname_, "does not exist (create_if_missing is false)"); } - } else { + } else if (s.ok()) { if (db_options_.error_if_exists) { return Status::InvalidArgument( dbname_, "exists (error_if_exists is true)"); } + } else { + // Unexpected error reading file + assert(s.IsIOError()); + return s; } // Check for the IDENTITY file and create it if not there - if (!env_->FileExists(IdentityFileName(dbname_))) { + s = env_->FileExists(IdentityFileName(dbname_)); + if (s.IsNotFound()) { s = SetIdentityFile(env_, dbname_); if (!s.ok()) { return s; } + } else if (!s.ok()) { + assert(s.IsIOError()); + return s; } } diff --git a/db/db_test.cc b/db/db_test.cc index b185b2bea..1ff4e7bcf 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -8450,13 +8450,13 @@ TEST_F(DBTest, DeleteMovedFileAfterCompaction) { ASSERT_EQ("0,0,2", FilesPerLevel(0)); // iterator is holding the file - ASSERT_TRUE(env_->FileExists(dbname_ + moved_file_name)); + ASSERT_OK(env_->FileExists(dbname_ + moved_file_name)); listener->SetExpectedFileName(dbname_ + moved_file_name); iterator.reset(); // this file should have been compacted away - ASSERT_TRUE(!env_->FileExists(dbname_ + moved_file_name)); + ASSERT_EQ(Status::NotFound(), env_->FileExists(dbname_ + moved_file_name)); listener->VerifyMatchedCount(1); } } @@ -8703,7 +8703,7 @@ TEST_F(DBTest, DeleteObsoleteFilesPendingOutputs) { ASSERT_EQ(metadata.size(), 2U); // This file should have been deleted during last compaction - ASSERT_TRUE(!env_->FileExists(dbname_ + file_on_L2)); + ASSERT_EQ(Status::NotFound(), env_->FileExists(dbname_ + file_on_L2)); listener->VerifyMatchedCount(1); } diff --git a/db/deletefile_test.cc b/db/deletefile_test.cc index 98c58e440..b4ddad5e2 100644 --- a/db/deletefile_test.cc +++ b/db/deletefile_test.cc @@ -269,11 +269,11 @@ TEST_F(DeleteFileTest, DeleteLogFiles) { // Should not succeed because live logs are not allowed to be deleted std::unique_ptr alive_log = std::move(logfiles.back()); ASSERT_EQ(alive_log->Type(), kAliveLogFile); - ASSERT_TRUE(env_->FileExists(options_.wal_dir + "/" + alive_log->PathName())); + ASSERT_OK(env_->FileExists(options_.wal_dir + "/" + alive_log->PathName())); fprintf(stdout, "Deleting alive log file %s\n", alive_log->PathName().c_str()); ASSERT_TRUE(!db_->DeleteFile(alive_log->PathName()).ok()); - ASSERT_TRUE(env_->FileExists(options_.wal_dir + "/" + alive_log->PathName())); + ASSERT_OK(env_->FileExists(options_.wal_dir + "/" + alive_log->PathName())); logfiles.clear(); // Call Flush to bring about a new working log file and add more keys @@ -287,13 +287,13 @@ TEST_F(DeleteFileTest, DeleteLogFiles) { ASSERT_GT(logfiles.size(), 0UL); std::unique_ptr archived_log = std::move(logfiles.front()); ASSERT_EQ(archived_log->Type(), kArchivedLogFile); - ASSERT_TRUE(env_->FileExists(options_.wal_dir + "/" + - archived_log->PathName())); + ASSERT_OK( + env_->FileExists(options_.wal_dir + "/" + archived_log->PathName())); fprintf(stdout, "Deleting archived log file %s\n", archived_log->PathName().c_str()); ASSERT_OK(db_->DeleteFile(archived_log->PathName())); - ASSERT_TRUE(!env_->FileExists(options_.wal_dir + "/" + - archived_log->PathName())); + ASSERT_EQ(Status::NotFound(), env_->FileExists(options_.wal_dir + "/" + + archived_log->PathName())); CloseDB(); } diff --git a/db/fault_injection_test.cc b/db/fault_injection_test.cc index 9cddea8b8..29d5d7387 100644 --- a/db/fault_injection_test.cc +++ b/db/fault_injection_test.cc @@ -191,10 +191,14 @@ class FaultInjectionTestEnv : public EnvWrapper { return Status::Corruption("Not Active"); } // Not allow overwriting files - if (target()->FileExists(fname)) { + Status s = target()->FileExists(fname); + if (s.ok()) { return Status::Corruption("File already exists."); + } else if (!s.IsNotFound()) { + assert(s.IsIOError()); + return s; } - Status s = target()->NewWritableFile(fname, result, soptions); + s = target()->NewWritableFile(fname, result, soptions); if (s.ok()) { result->reset(new TestWritableFile(fname, std::move(*result), this)); // WritableFileWriter* file is opened diff --git a/db/wal_manager.cc b/db/wal_manager.cc index dceaf6643..37861ab45 100644 --- a/db/wal_manager.cc +++ b/db/wal_manager.cc @@ -59,11 +59,15 @@ Status WalManager::GetSortedWalFiles(VectorLogPtr& files) { files.clear(); // list wal files in archive dir. std::string archivedir = ArchivalDirectory(db_options_.wal_dir); - if (env_->FileExists(archivedir)) { + Status exists = env_->FileExists(archivedir); + if (exists.ok()) { s = GetSortedWalsOfType(archivedir, files, kArchivedLogFile); if (!s.ok()) { return s; } + } else if (!exists.IsNotFound()) { + assert(s.IsIOError()); + return s; } uint64_t latest_archived_log_number = 0; @@ -313,9 +317,9 @@ Status WalManager::GetSortedWalsOfType(const std::string& path, // re-try in case the alive log file has been moved to archive. std::string archived_file = ArchivedLogFileName(path, number); if (!s.ok() && log_type == kAliveLogFile && - env_->FileExists(archived_file)) { + env_->FileExists(archived_file).ok()) { s = env_->GetFileSize(archived_file, &size_bytes); - if (!s.ok() && !env_->FileExists(archived_file)) { + if (!s.ok() && env_->FileExists(archived_file).IsNotFound()) { // oops, the file just got deleted from archived dir! move on s = Status::OK(); continue; @@ -380,7 +384,7 @@ Status WalManager::ReadFirstRecord(const WalFileType type, if (type == kAliveLogFile) { std::string fname = LogFileName(db_options_.wal_dir, number); s = ReadFirstLine(fname, sequence); - if (env_->FileExists(fname) && !s.ok()) { + if (env_->FileExists(fname).ok() && !s.ok()) { // return any error that is not caused by non-existing file return s; } @@ -394,7 +398,7 @@ Status WalManager::ReadFirstRecord(const WalFileType type, // maybe the file was deleted from archive dir. If that's the case, return // Status::OK(). The caller with identify this as empty file because // *sequence == 0 - if (!s.ok() && !env_->FileExists(archived_file)) { + if (!s.ok() && env_->FileExists(archived_file).IsNotFound()) { return Status::OK(); } } diff --git a/hdfs/env_hdfs.h b/hdfs/env_hdfs.h index db6cffa02..efa63e53c 100644 --- a/hdfs/env_hdfs.h +++ b/hdfs/env_hdfs.h @@ -73,7 +73,7 @@ class HdfsEnv : public Env { virtual Status NewDirectory(const std::string& name, std::unique_ptr* result); - virtual bool FileExists(const std::string& fname); + virtual Status FileExists(const std::string& fname); virtual Status GetChildren(const std::string& path, std::vector* result); @@ -279,7 +279,9 @@ class HdfsEnv : public Env { return notsup; } - virtual bool FileExists(const std::string& fname) override { return false; } + virtual Status FileExists(const std::string& fname) override { + return notsup; + } virtual Status GetChildren(const std::string& path, std::vector* result) override { diff --git a/include/rocksdb/env.h b/include/rocksdb/env.h index acdc66e48..2be261e42 100644 --- a/include/rocksdb/env.h +++ b/include/rocksdb/env.h @@ -156,8 +156,12 @@ class Env { virtual Status NewDirectory(const std::string& name, unique_ptr* result) = 0; - // Returns true iff the named file exists. - virtual bool FileExists(const std::string& fname) = 0; + // Returns OK if the named file exists. + // NotFound if the named file does not exist, + // the calling process does not have permission to determine + // whether this file exists, or if the path is invalid. + // IOError if an IO Error was encountered + virtual Status FileExists(const std::string& fname) = 0; // Store in *result the names of the children of the specified directory. // The names are relative to "dir". @@ -764,7 +768,7 @@ class EnvWrapper : public Env { unique_ptr* result) override { return target_->NewDirectory(name, result); } - bool FileExists(const std::string& f) override { + Status FileExists(const std::string& f) override { return target_->FileExists(f); } Status GetChildren(const std::string& dir, diff --git a/include/rocksdb/status.h b/include/rocksdb/status.h index 9fe0c9a85..d99fabac0 100644 --- a/include/rocksdb/status.h +++ b/include/rocksdb/status.h @@ -31,6 +31,8 @@ class Status { // Copy the specified status. Status(const Status& s); void operator=(const Status& s); + bool operator==(const Status& rhs) const; + bool operator!=(const Status& rhs) const; // Return a success status. static Status OK() { return Status(); } @@ -164,6 +166,14 @@ inline void Status::operator=(const Status& s) { } } +inline bool Status::operator==(const Status& rhs) const { + return (code_ == rhs.code_); +} + +inline bool Status::operator!=(const Status& rhs) const { + return !(*this == rhs); +} + } // namespace rocksdb #endif // STORAGE_ROCKSDB_INCLUDE_STATUS_H_ diff --git a/port/win/env_win.cc b/port/win/env_win.cc index ce9dcd4e0..2890ecc49 100644 --- a/port/win/env_win.cc +++ b/port/win/env_win.cc @@ -1663,10 +1663,11 @@ class WinEnv : public Env { return s; } - virtual bool FileExists(const std::string& fname) override { + virtual Status FileExists(const std::string& fname) override { // F_OK == 0 const int F_OK_ = 0; - return _access(fname.c_str(), F_OK_) == 0; + return _access(fname.c_str(), F_OK_) == 0 ? Status::OK() + : Status::NotFound(); } virtual Status GetChildren(const std::string& dir, diff --git a/table/cuckoo_table_reader_test.cc b/table/cuckoo_table_reader_test.cc index 87db2326d..c4a3d5007 100644 --- a/table/cuckoo_table_reader_test.cc +++ b/table/cuckoo_table_reader_test.cc @@ -522,7 +522,8 @@ TEST_F(CuckooReaderTest, TestReadPerformance) { "WARNING: Not compiled with DNDEBUG. Performance tests may be slow.\n"); #endif for (uint64_t num : nums) { - if (FLAGS_write || !Env::Default()->FileExists(GetFileName(num))) { + if (FLAGS_write || + Env::Default()->FileExists(GetFileName(num)).IsNotFound()) { std::vector all_keys; GetKeys(num, &all_keys); WriteFile(all_keys, num, hash_ratio); diff --git a/util/auto_roll_logger_test.cc b/util/auto_roll_logger_test.cc index 1e46594df..cbcc4cf28 100644 --- a/util/auto_roll_logger_test.cc +++ b/util/auto_roll_logger_test.cc @@ -155,9 +155,9 @@ TEST_F(AutoRollLoggerTest, RollLogFileByTime) { InitTestDb(); // -- Test the existence of file during the server restart. - ASSERT_TRUE(!env->FileExists(kLogFile)); + ASSERT_EQ(Status::NotFound(), env->FileExists(kLogFile)); AutoRollLogger logger(Env::Default(), kTestDir, "", log_size, time); - ASSERT_TRUE(env->FileExists(kLogFile)); + ASSERT_OK(env->FileExists(kLogFile)); RollLogFileByTimeTest(&logger, time, kSampleMessage + ":RollLogFileByTime"); } @@ -397,7 +397,7 @@ TEST_F(AutoRollLoggerTest, LogFileExistence) { options.max_log_file_size = 100 * 1024 * 1024; options.create_if_missing = true; ASSERT_OK(rocksdb::DB::Open(options, kTestDir, &db)); - ASSERT_TRUE(env->FileExists(kLogFile)); + ASSERT_OK(env->FileExists(kLogFile)); delete db; } diff --git a/util/env_hdfs.cc b/util/env_hdfs.cc index 298eb48fa..e0016c66f 100644 --- a/util/env_hdfs.cc +++ b/util/env_hdfs.cc @@ -448,20 +448,18 @@ Status HdfsEnv::NewDirectory(const std::string& name, } } -bool HdfsEnv::FileExists(const std::string& fname) { - +Status HdfsEnv::FileExists(const std::string& fname) { int value = hdfsExists(fileSys_, fname.c_str()); switch (value) { case HDFS_EXISTS: - return true; + return Status::OK(); case HDFS_DOESNT_EXIST: - return false; + return Status::NotFound(); default: // anything else should be an error Log(InfoLogLevel::FATAL_LEVEL, mylog, "FileExists hdfsExists call failed"); - throw HdfsFatalException("hdfsExists call failed with error " + - ToString(value) + " on path " + fname + - ".\n"); + return Status::IOError("hdfsExists call failed with error " + + ToString(value) + " on path " + fname + ".\n"); } } diff --git a/util/env_posix.cc b/util/env_posix.cc index 978d3d505..80ef54566 100644 --- a/util/env_posix.cc +++ b/util/env_posix.cc @@ -40,6 +40,7 @@ #include "util/posix_logger.h" #include "util/random.h" #include "util/iostats_context_imp.h" +#include "util/string_util.h" #include "util/sync_point.h" #include "util/thread_status_updater.h" #include "util/thread_status_util.h" @@ -1055,8 +1056,25 @@ class PosixEnv : public Env { return Status::OK(); } - virtual bool FileExists(const std::string& fname) override { - return access(fname.c_str(), F_OK) == 0; + virtual Status FileExists(const std::string& fname) override { + int result = access(fname.c_str(), F_OK); + + if (result == 0) { + return Status::OK(); + } + + switch (errno) { + case EACCES: + case ELOOP: + case ENAMETOOLONG: + case ENOENT: + case ENOTDIR: + return Status::NotFound(); + default: + assert(result == EIO || result == ENOMEM); + return Status::IOError("Unexpected error(" + ToString(result) + + ") accessing file `" + fname + "' "); + } } virtual Status GetChildren(const std::string& dir, @@ -1772,9 +1790,11 @@ void PosixEnv::WaitForJoin() { std::string Env::GenerateUniqueId() { std::string uuid_file = "/proc/sys/kernel/random/uuid"; - if (FileExists(uuid_file)) { + + Status s = FileExists(uuid_file); + if (s.ok()) { std::string uuid; - Status s = ReadFileToString(this, uuid_file, &uuid); + s = ReadFileToString(this, uuid_file, &uuid); if (s.ok()) { return uuid; } diff --git a/util/memenv.cc b/util/memenv.cc index c89411f7e..72fdbd242 100644 --- a/util/memenv.cc +++ b/util/memenv.cc @@ -308,10 +308,14 @@ class InMemoryEnv : public EnvWrapper { return Status::OK(); } - virtual bool FileExists(const std::string& fname) override { + virtual Status FileExists(const std::string& fname) override { std::string nfname = NormalizeFileName(fname); MutexLock lock(&mutex_); - return file_map_.find(nfname) != file_map_.end(); + if (file_map_.find(nfname) != file_map_.end()) { + return Status::OK(); + } else { + return Status::NotFound(); + } } virtual Status GetChildren(const std::string& dir, diff --git a/util/memenv_test.cc b/util/memenv_test.cc index ee8c41a7b..24190daba 100644 --- a/util/memenv_test.cc +++ b/util/memenv_test.cc @@ -35,7 +35,7 @@ TEST_F(MemEnvTest, Basics) { ASSERT_OK(env_->CreateDir("/dir")); // Check that the directory is empty. - ASSERT_TRUE(!env_->FileExists("/dir/non_existent")); + ASSERT_EQ(Status::NotFound(), env_->FileExists("/dir/non_existent")); ASSERT_TRUE(!env_->GetFileSize("/dir/non_existent", &file_size).ok()); ASSERT_OK(env_->GetChildren("/dir", &children)); ASSERT_EQ(0U, children.size()); @@ -45,7 +45,7 @@ TEST_F(MemEnvTest, Basics) { writable_file.reset(); // Check that the file exists. - ASSERT_TRUE(env_->FileExists("/dir/f")); + ASSERT_OK(env_->FileExists("/dir/f")); ASSERT_OK(env_->GetFileSize("/dir/f", &file_size)); ASSERT_EQ(0U, file_size); ASSERT_OK(env_->GetChildren("/dir", &children)); @@ -64,8 +64,8 @@ TEST_F(MemEnvTest, Basics) { // Check that renaming works. ASSERT_TRUE(!env_->RenameFile("/dir/non_existent", "/dir/g").ok()); ASSERT_OK(env_->RenameFile("/dir/f", "/dir/g")); - ASSERT_TRUE(!env_->FileExists("/dir/f")); - ASSERT_TRUE(env_->FileExists("/dir/g")); + ASSERT_EQ(Status::NotFound(), env_->FileExists("/dir/f")); + ASSERT_OK(env_->FileExists("/dir/g")); ASSERT_OK(env_->GetFileSize("/dir/g", &file_size)); ASSERT_EQ(3U, file_size); @@ -82,7 +82,7 @@ TEST_F(MemEnvTest, Basics) { // Check that deleting works. ASSERT_TRUE(!env_->DeleteFile("/dir/non_existent").ok()); ASSERT_OK(env_->DeleteFile("/dir/g")); - ASSERT_TRUE(!env_->FileExists("/dir/g")); + ASSERT_EQ(Status::NotFound(), env_->FileExists("/dir/g")); ASSERT_OK(env_->GetChildren("/dir", &children)); ASSERT_EQ(0U, children.size()); ASSERT_OK(env_->DeleteDir("/dir")); diff --git a/util/mock_env.cc b/util/mock_env.cc index 716d137d4..3e62c040c 100644 --- a/util/mock_env.cc +++ b/util/mock_env.cc @@ -468,12 +468,12 @@ Status MockEnv::NewDirectory(const std::string& name, return Status::OK(); } -bool MockEnv::FileExists(const std::string& fname) { +Status MockEnv::FileExists(const std::string& fname) { auto fn = NormalizePath(fname); MutexLock lock(&mutex_); if (file_map_.find(fn) != file_map_.end()) { // File exists - return true; + return Status::OK(); } // Now also check if fn exists as a dir for (const auto& iter : file_map_) { @@ -481,10 +481,10 @@ bool MockEnv::FileExists(const std::string& fname) { if (filename.size() >= fn.size() + 1 && filename[fn.size()] == '/' && Slice(filename).starts_with(Slice(fn))) { - return true; + return Status::OK(); } } - return false; + return Status::NotFound(); } Status MockEnv::GetChildren(const std::string& dir, diff --git a/util/mock_env.h b/util/mock_env.h index 55ef24b67..4e10e7eac 100644 --- a/util/mock_env.h +++ b/util/mock_env.h @@ -46,7 +46,7 @@ class MockEnv : public EnvWrapper { virtual Status NewDirectory(const std::string& name, unique_ptr* result) override; - virtual bool FileExists(const std::string& fname) override; + virtual Status FileExists(const std::string& fname) override; virtual Status GetChildren(const std::string& dir, std::vector* result) override; diff --git a/util/mock_env_test.cc b/util/mock_env_test.cc index 12e289059..2f50c2a82 100644 --- a/util/mock_env_test.cc +++ b/util/mock_env_test.cc @@ -34,7 +34,7 @@ TEST_F(MockEnvTest, Basics) { ASSERT_OK(env_->CreateDir("/dir")); // Check that the directory is empty. - ASSERT_TRUE(!env_->FileExists("/dir/non_existent")); + ASSERT_EQ(Status::NotFound(), env_->FileExists("/dir/non_existent")); ASSERT_TRUE(!env_->GetFileSize("/dir/non_existent", &file_size).ok()); ASSERT_OK(env_->GetChildren("/dir", &children)); ASSERT_EQ(0U, children.size()); @@ -44,7 +44,7 @@ TEST_F(MockEnvTest, Basics) { writable_file.reset(); // Check that the file exists. - ASSERT_TRUE(env_->FileExists("/dir/f")); + ASSERT_OK(env_->FileExists("/dir/f")); ASSERT_OK(env_->GetFileSize("/dir/f", &file_size)); ASSERT_EQ(0U, file_size); ASSERT_OK(env_->GetChildren("/dir", &children)); @@ -63,8 +63,8 @@ TEST_F(MockEnvTest, Basics) { // Check that renaming works. ASSERT_TRUE(!env_->RenameFile("/dir/non_existent", "/dir/g").ok()); ASSERT_OK(env_->RenameFile("/dir/f", "/dir/g")); - ASSERT_TRUE(!env_->FileExists("/dir/f")); - ASSERT_TRUE(env_->FileExists("/dir/g")); + ASSERT_EQ(Status::NotFound(), env_->FileExists("/dir/f")); + ASSERT_OK(env_->FileExists("/dir/g")); ASSERT_OK(env_->GetFileSize("/dir/g", &file_size)); ASSERT_EQ(3U, file_size); @@ -81,7 +81,7 @@ TEST_F(MockEnvTest, Basics) { // Check that deleting works. ASSERT_TRUE(!env_->DeleteFile("/dir/non_existent").ok()); ASSERT_OK(env_->DeleteFile("/dir/g")); - ASSERT_TRUE(!env_->FileExists("/dir/g")); + ASSERT_EQ(Status::NotFound(), env_->FileExists("/dir/g")); ASSERT_OK(env_->GetChildren("/dir", &children)); ASSERT_EQ(0U, children.size()); ASSERT_OK(env_->DeleteDir("/dir")); diff --git a/utilities/backupable/backupable_db.cc b/utilities/backupable/backupable_db.cc index c25cec936..e88326952 100644 --- a/utilities/backupable/backupable_db.cc +++ b/utilities/backupable/backupable_db.cc @@ -1117,10 +1117,12 @@ Status BackupEngineImpl::GetLatestBackupFileContents(uint32_t* latest_backup) { *latest_backup = 0; sscanf(data.data(), "%u", latest_backup); - if (backup_env_->FileExists(GetBackupMetaFile(*latest_backup)) == false) { + + s = backup_env_->FileExists(GetBackupMetaFile(*latest_backup)); + if (s.IsNotFound()) { s = Status::Corruption("Latest backup file corrupted"); } - return Status::OK(); + return s; } // this operation HAS to be atomic @@ -1283,7 +1285,21 @@ Status BackupEngineImpl::AddBackupFileWorkItem( // true if dst_path is the same path as another live file const bool same_path = live_dst_paths.find(dst_path) != live_dst_paths.end(); - if (shared && (backup_env_->FileExists(dst_path) || same_path)) { + + bool file_exists = false; + if (shared && !same_path) { + Status exist = backup_env_->FileExists(dst_path); + if (exist.ok()) { + file_exists = true; + } else if (exist.IsNotFound()) { + file_exists = false; + } else { + assert(s.IsIOError()); + return exist; + } + } + + if (shared && (same_path || file_exists)) { need_to_copy = false; if (shared_checksum) { Log(options_.info_log, @@ -1510,8 +1526,13 @@ Status BackupEngineImpl::BackupMeta::Delete(bool delete_meta) { } files_.clear(); // delete meta file - if (delete_meta && env_->FileExists(meta_filename_)) { - s = env_->DeleteFile(meta_filename_); + if (delete_meta) { + s = env_->FileExists(meta_filename_); + if (s.ok()) { + s = env_->DeleteFile(meta_filename_); + } else if (s.IsNotFound()) { + s = Status::OK(); // nothing to delete + } } timestamp_ = 0; return s; diff --git a/utilities/backupable/backupable_db_test.cc b/utilities/backupable/backupable_db_test.cc index fd2bf87f3..8f2bd4ec2 100644 --- a/utilities/backupable/backupable_db_test.cc +++ b/utilities/backupable/backupable_db_test.cc @@ -584,10 +584,12 @@ TEST_F(BackupableDBTest, NoDoubleCopy) { test_backup_env_->AssertWrittenFiles(should_have_written); ASSERT_OK(backup_engine_->DeleteBackup(1)); - ASSERT_TRUE(test_backup_env_->FileExists(backupdir_ + "/shared/00010.sst")); + ASSERT_OK(test_backup_env_->FileExists(backupdir_ + "/shared/00010.sst")); + // 00011.sst was only in backup 1, should be deleted - ASSERT_FALSE(test_backup_env_->FileExists(backupdir_ + "/shared/00011.sst")); - ASSERT_TRUE(test_backup_env_->FileExists(backupdir_ + "/shared/00015.sst")); + ASSERT_EQ(Status::NotFound(), + test_backup_env_->FileExists(backupdir_ + "/shared/00011.sst")); + ASSERT_OK(test_backup_env_->FileExists(backupdir_ + "/shared/00015.sst")); // MANIFEST file size should be only 100 uint64_t size; @@ -678,8 +680,10 @@ TEST_F(BackupableDBTest, CorruptionsTest) { 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_TRUE(file_manager_->FileExists(backupdir_ + "/meta/6") == false); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/private/6") == false); + ASSERT_EQ(Status::NotFound(), + file_manager_->FileExists(backupdir_ + "/meta/6")); + ASSERT_EQ(Status::NotFound(), + file_manager_->FileExists(backupdir_ + "/private/6")); // --------- case 3. corrupted backup meta or missing backuped file ---- ASSERT_OK(file_manager_->CorruptFile(backupdir_ + "/meta/5", 3)); @@ -705,23 +709,23 @@ TEST_F(BackupableDBTest, CorruptionsTest) { // checksum of the backup 2 appears to be valid, this can cause checksum // mismatch and abort restore process ASSERT_OK(file_manager_->CorruptChecksum(backupdir_ + "/meta/2", true)); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/2")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/meta/2")); OpenBackupEngine(); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/2")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/meta/2")); s = backup_engine_->RestoreDBFromBackup(2, dbname_, dbname_); ASSERT_TRUE(!s.ok()); // make sure that no corrupt backups have actually been deleted! - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/1")); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/2")); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/3")); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/4")); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/5")); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/private/1")); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/private/2")); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/private/3")); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/private/4")); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/private/5")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/meta/1")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/meta/2")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/meta/3")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/meta/4")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/meta/5")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/private/1")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/private/2")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/private/3")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/private/4")); + ASSERT_OK(file_manager_->FileExists(backupdir_ + "/private/5")); // delete the corrupt backups and then make sure they're actually deleted ASSERT_OK(backup_engine_->DeleteBackup(5)); @@ -729,14 +733,22 @@ TEST_F(BackupableDBTest, CorruptionsTest) { ASSERT_OK(backup_engine_->DeleteBackup(3)); ASSERT_OK(backup_engine_->DeleteBackup(2)); (void)backup_engine_->GarbageCollect(); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/5") == false); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/private/5") == false); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/4") == false); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/private/4") == false); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/3") == false); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/private/3") == false); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/2") == false); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/private/2") == false); + ASSERT_EQ(Status::NotFound(), + file_manager_->FileExists(backupdir_ + "/meta/5")); + ASSERT_EQ(Status::NotFound(), + file_manager_->FileExists(backupdir_ + "/private/5")); + ASSERT_EQ(Status::NotFound(), + file_manager_->FileExists(backupdir_ + "/meta/4")); + ASSERT_EQ(Status::NotFound(), + file_manager_->FileExists(backupdir_ + "/private/4")); + ASSERT_EQ(Status::NotFound(), + file_manager_->FileExists(backupdir_ + "/meta/3")); + ASSERT_EQ(Status::NotFound(), + file_manager_->FileExists(backupdir_ + "/private/3")); + ASSERT_EQ(Status::NotFound(), + file_manager_->FileExists(backupdir_ + "/meta/2")); + ASSERT_EQ(Status::NotFound(), + file_manager_->FileExists(backupdir_ + "/private/2")); CloseBackupEngine(); AssertBackupConsistency(0, 0, keys_iteration * 1, keys_iteration * 5); @@ -772,8 +784,8 @@ TEST_F(BackupableDBTest, NoDeleteWithReadOnly) { // assert that data from backup 5 is still here (even though LATEST_BACKUP // says 4 is latest) - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/meta/5") == true); - ASSERT_TRUE(file_manager_->FileExists(backupdir_ + "/private/5") == true); + 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 std::vector backup_info; @@ -997,14 +1009,14 @@ TEST_F(BackupableDBTest, DeleteTmpFiles) { file_manager_->WriteToFile(shared_tmp, "tmp"); file_manager_->CreateDir(private_tmp_dir); file_manager_->WriteToFile(private_tmp_file, "tmp"); - ASSERT_TRUE(file_manager_->FileExists(private_tmp_dir)); + ASSERT_OK(file_manager_->FileExists(private_tmp_dir)); OpenDBAndBackupEngine(); // Need to call this explicitly to delete tmp files (void)backup_engine_->GarbageCollect(); CloseDBAndBackupEngine(); - ASSERT_FALSE(file_manager_->FileExists(shared_tmp)); - ASSERT_FALSE(file_manager_->FileExists(private_tmp_file)); - ASSERT_FALSE(file_manager_->FileExists(private_tmp_dir)); + ASSERT_EQ(Status::NotFound(), file_manager_->FileExists(shared_tmp)); + ASSERT_EQ(Status::NotFound(), file_manager_->FileExists(private_tmp_file)); + ASSERT_EQ(Status::NotFound(), file_manager_->FileExists(private_tmp_dir)); } TEST_F(BackupableDBTest, KeepLogFiles) { diff --git a/utilities/checkpoint/checkpoint.cc b/utilities/checkpoint/checkpoint.cc index f9683e6e7..1bc44859f 100644 --- a/utilities/checkpoint/checkpoint.cc +++ b/utilities/checkpoint/checkpoint.cc @@ -64,8 +64,12 @@ Status CheckpointImpl::CreateCheckpoint(const std::string& checkpoint_dir) { bool same_fs = true; VectorLogPtr live_wal_files; - if (db_->GetEnv()->FileExists(checkpoint_dir)) { + s = db_->GetEnv()->FileExists(checkpoint_dir); + if (s.ok()) { return Status::InvalidArgument("Directory exists"); + } else if (!s.IsNotFound()) { + assert(s.IsIOError()); + return s; } s = db_->DisableFileDeletions();