From d1c9ede1956a29472fbe7202cd3e8ee7aefa7c31 Mon Sep 17 00:00:00 2001 From: Zhongyi Xie Date: Thu, 1 Aug 2019 15:45:19 -0700 Subject: [PATCH] Fix duplicated file names in PurgeObsoleteFiles (#5603) Summary: Currently in `DBImpl::PurgeObsoleteFiles`, the list of candidate files is create through a combination of calling LogFileName using `log_delete_files` and `full_scan_candidate_files`. In full_scan_candidate_files, the filenames look like this {file_name = "074715.log", file_path = "/txlogs/3306"}, but LogFileName produces filenames like this that prepends a slash: {file_name = "/074715.log", file_path = "/txlogs/3306"}, This confuses the dedup step here: https://github.com/facebook/rocksdb/blob/bb4178066dc4f18b9b7f1d371e641db027b3edbe/db/db_impl/db_impl_files.cc#L339-L345 Because duplicates still exist, DeleteFile is called on the same file twice, and hits an error on the second try. Error message: Failed to mark /txlogs/3302/764418.log as trash. The root cause is the use of `kDumbDbName` when generating file names, it creates file names like /074715.log. This PR removes the use of `kDumbDbName` and create paths without leading '/' when dbname can be ignored. Pull Request resolved: https://github.com/facebook/rocksdb/pull/5603 Test Plan: make check Differential Revision: D16413203 Pulled By: miasantreble fbshipit-source-id: 6ba8288382c55f7d5e3892d722fc94b57d2e4491 --- db/db_impl/db_impl_files.cc | 5 ++--- file/filename.cc | 21 +++++++++++++++++---- file/filename.h | 4 ++++ 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/db/db_impl/db_impl_files.cc b/db/db_impl/db_impl_files.cc index 7afe3955e..e3b2f5765 100644 --- a/db/db_impl/db_impl_files.cc +++ b/db/db_impl/db_impl_files.cc @@ -316,10 +316,9 @@ void DBImpl::PurgeObsoleteFiles(JobContext& state, bool schedule_only) { candidate_files.size() + state.sst_delete_files.size() + state.log_delete_files.size() + state.manifest_delete_files.size()); // We may ignore the dbname when generating the file names. - const char* kDumbDbName = ""; for (auto& file : state.sst_delete_files) { candidate_files.emplace_back( - MakeTableFileName(kDumbDbName, file.metadata->fd.GetNumber()), + MakeTableFileName(file.metadata->fd.GetNumber()), file.path); if (file.metadata->table_reader_handle) { table_cache_->Release(file.metadata->table_reader_handle); @@ -329,7 +328,7 @@ void DBImpl::PurgeObsoleteFiles(JobContext& state, bool schedule_only) { for (auto file_num : state.log_delete_files) { if (file_num > 0) { - candidate_files.emplace_back(LogFileName(kDumbDbName, file_num), + candidate_files.emplace_back(LogFileName(file_num), immutable_db_options_.wal_dir); } } diff --git a/file/filename.cc b/file/filename.cc index d4f7dd9ec..65ec33149 100644 --- a/file/filename.cc +++ b/file/filename.cc @@ -57,13 +57,17 @@ static size_t GetInfoLogPrefix(const std::string& path, char* dest, int len) { return write_idx; } -static std::string MakeFileName(const std::string& name, uint64_t number, - const char* suffix) { +static std::string MakeFileName(uint64_t number, const char* suffix) { char buf[100]; - snprintf(buf, sizeof(buf), "/%06llu.%s", + snprintf(buf, sizeof(buf), "%06llu.%s", static_cast(number), suffix); - return name + buf; + return buf; +} + +static std::string MakeFileName(const std::string& name, uint64_t number, + const char* suffix) { + return name + "/" + MakeFileName(number, suffix); } std::string LogFileName(const std::string& name, uint64_t number) { @@ -71,6 +75,11 @@ std::string LogFileName(const std::string& name, uint64_t number) { return MakeFileName(name, number, "log"); } +std::string LogFileName(uint64_t number) { + assert(number > 0); + return MakeFileName(number, "log"); +} + std::string BlobFileName(const std::string& blobdirname, uint64_t number) { assert(number > 0); return MakeFileName(blobdirname, number, kRocksDBBlobFileExt.c_str()); @@ -95,6 +104,10 @@ std::string MakeTableFileName(const std::string& path, uint64_t number) { return MakeFileName(path, number, kRocksDbTFileExt.c_str()); } +std::string MakeTableFileName(uint64_t number) { + return MakeFileName(number, kRocksDbTFileExt.c_str()); +} + std::string Rocks2LevelTableFileName(const std::string& fullname) { assert(fullname.size() > kRocksDbTFileExt.size() + 1); if (fullname.size() <= kRocksDbTFileExt.size() + 1) { diff --git a/file/filename.h b/file/filename.h index db06f4664..91b905f07 100644 --- a/file/filename.h +++ b/file/filename.h @@ -47,6 +47,8 @@ enum FileType { // "dbname". extern std::string LogFileName(const std::string& dbname, uint64_t number); +extern std::string LogFileName(uint64_t number); + extern std::string BlobFileName(const std::string& bdirname, uint64_t number); extern std::string BlobFileName(const std::string& dbname, @@ -63,6 +65,8 @@ extern std::string ArchivedLogFileName(const std::string& dbname, extern std::string MakeTableFileName(const std::string& name, uint64_t number); +extern std::string MakeTableFileName(uint64_t number); + // Return the name of sstable with LevelDB suffix // created from RocksDB sstable suffixed name extern std::string Rocks2LevelTableFileName(const std::string& fullname);