Unify DeleteFile and DeleteWalFiles

Summary:
This is to simplify rocksdb public APIs and improve the code quality.
Created an additional parameter to ParseFileName for log sub type and improved the code for deleting a wal file.
Wrote exhaustive unit-tests in delete_file_test
Unification of other redundant APIs can be taken up in a separate diff

Test Plan: Expanded delete_file test

Reviewers: dhruba, haobo, kailiu, sdong

Reviewed By: dhruba

CC: leveldb

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

db/db_filesnapshot.cc

@@ -91,29 +91,4 @@ Status DBImpl::GetSortedWalFiles(VectorLogPtr& files) {
   return AppendSortedWalsOfType(options_.wal_dir, files, kAliveLogFile);
 }
 
-Status DBImpl::DeleteWalFiles(const VectorLogPtr& files) {
-  Status s;
-  std::string archivedir = ArchivalDirectory(options_.wal_dir);
-  std::string files_not_deleted;
-  for (const auto& wal : files) {
-    /* Try deleting in the dir that pathname points to for the logfile.
-       This may fail if we try to delete a log file which was live when captured
-       but is archived now. Try deleting it from archive also
-     */
-    Status st = env_->DeleteFile(options_.wal_dir + "/" + wal->PathName());
-    if (!st.ok()) {
-      if (wal->Type() == kAliveLogFile &&
-          env_->DeleteFile(LogFileName(archivedir, wal->LogNumber())).ok()) {
-        continue;
-      }
-      files_not_deleted.append(wal->PathName());
-    }
-  }
-  if (!files_not_deleted.empty()) {
-    return Status::IOError("Deleted all requested files except: " +
-                           files_not_deleted);
-  }
-  return Status::OK();
-}
-
 }

db/db_impl.cc

@@ -3149,26 +3149,38 @@ inline void DBImpl::DelayLoggingAndReset() {
 Status DBImpl::DeleteFile(std::string name) {
   uint64_t number;
   FileType type;
-  if (!ParseFileName(name, &number, &type) ||
-      (type != kTableFile)) {
-    Log(options_.info_log,
-        "DeleteFile #%ld FAILED. Invalid file name\n",
-        number);
+  WalFileType log_type;
+  if (!ParseFileName(name, &number, &type, &log_type) ||
+      (type != kTableFile && type != kLogFile)) {
+    Log(options_.info_log, "DeleteFile %s failed.\n", name.c_str());
     return Status::InvalidArgument("Invalid file name");
   }
 
+  Status status;
+  if (type == kLogFile) {
+    // Only allow deleting archived log files
+    if (log_type != kArchivedLogFile) {
+      Log(options_.info_log, "DeleteFile %s failed.\n", name.c_str());
+      return Status::NotSupported("Delete only supported for archived logs");
+    }
+    status = env_->DeleteFile(options_.wal_dir + "/" + name.c_str());
+    if (!status.ok()) {
+      Log(options_.info_log, "DeleteFile %s failed.\n", name.c_str());
+    }
+    return status;
+  }
+
   int level;
   FileMetaData metadata;
   int maxlevel = NumberLevels();
   VersionEdit edit(maxlevel);
   DeletionState deletion_state;
-  Status status;
   {
     MutexLock l(&mutex_);
     status = versions_->GetMetadataForFile(number, &level, &metadata);
     if (!status.ok()) {
-      Log(options_.info_log, "DeleteFile #%lld FAILED. File not found\n",
-          static_cast<unsigned long long>(number));
+      Log(options_.info_log, "DeleteFile %s failed. File not found\n",
+                             name.c_str());
       return Status::InvalidArgument("File not found");
     }
     assert((level > 0) && (level < maxlevel));
@@ -3176,8 +3188,7 @@ Status DBImpl::DeleteFile(std::string name) {
     // If the file is being compacted no need to delete.
     if (metadata.being_compacted) {
       Log(options_.info_log,
-          "DeleteFile #%lld Skipped. File about to be compacted\n",
-          static_cast<unsigned long long>(number));
+          "DeleteFile %s Skipped. File about to be compacted\n", name.c_str());
       return Status::OK();
     }
 
@@ -3187,8 +3198,7 @@ Status DBImpl::DeleteFile(std::string name) {
     for (int i = level + 1; i < maxlevel; i++) {
       if (versions_->NumLevelFiles(i) != 0) {
         Log(options_.info_log,
-            "DeleteFile #%lld FAILED. File not in last level\n",
-            static_cast<unsigned long long>(number));
+            "DeleteFile %s FAILED. File not in last level\n", name.c_str());
         return Status::InvalidArgument("File not in last level");
       }
     }

db/db_impl.h

@@ -73,7 +73,6 @@ class DBImpl : public DB {
                               uint64_t* manifest_file_size,
                               bool flush_memtable = true);
   virtual Status GetSortedWalFiles(VectorLogPtr& files);
-  virtual Status DeleteWalFiles(const VectorLogPtr& files);
   virtual SequenceNumber GetLatestSequenceNumber();
   virtual Status GetUpdatesSince(SequenceNumber seq_number,
                                  unique_ptr<TransactionLogIterator>* iter);

db/db_test.cc

@@ -4104,7 +4104,7 @@ class ModelDB: public DB {
     return Status::OK();
   }
 
-  virtual Status DeleteWalFiles(const VectorLogPtr& files) {
+  virtual Status DeleteFile(std::string name) {
     return Status::OK();
   }
 

db/deletefile_test.cc

@@ -15,6 +15,7 @@
 #include "util/testharness.h"
 #include "util/testutil.h"
 #include "rocksdb/env.h"
+#include "rocksdb/transaction_log.h"
 #include <vector>
 #include <stdlib.h>
 #include <map>
@@ -36,6 +37,7 @@ class DeleteFileTest {
     options_.write_buffer_size = 1024*1024*1000;
     options_.target_file_size_base = 1024*1024*1000;
     options_.max_bytes_for_level_base = 1024*1024*1000;
+    options_.WAL_ttl_seconds = 300; // Used to test log files
     dbname_ = test::TmpDir() + "/deletefile_test";
     DestroyDB(dbname_, options_);
     numlevels_ = 7;
@@ -153,7 +155,6 @@ TEST(DeleteFileTest, AddKeysAndQueryLevels) {
   CloseDB();
 }
 
-
 TEST(DeleteFileTest, DeleteFileWithIterator) {
   CreateTwoLevels();
   ReadOptions options;
@@ -184,6 +185,41 @@ TEST(DeleteFileTest, DeleteFileWithIterator) {
   delete it;
   CloseDB();
 }
+
+TEST(DeleteFileTest, DeleteLogFiles) {
+  AddKeys(10, 0);
+  VectorLogPtr logfiles;
+  db_->GetSortedWalFiles(logfiles);
+  ASSERT_GT(logfiles.size(), 0UL);
+  // Take the last log file which is expected to be alive and try to delete it
+  // Should not succeed because live logs are not allowed to be deleted
+  std::unique_ptr<LogFile> alive_log = std::move(logfiles.back());
+  ASSERT_EQ(alive_log->Type(), kAliveLogFile);
+  ASSERT_TRUE(env_->FileExists(dbname_ + "/" + 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(dbname_ + "/" + alive_log->PathName()));
+  logfiles.clear();
+
+  // Call Flush to bring about a new working log file and add more keys
+  // Call Flush again to flush out memtable and move alive log to archived log
+  // and try to delete the archived log file
+  FlushOptions fopts;
+  db_->Flush(fopts);
+  AddKeys(10, 0);
+  db_->Flush(fopts);
+  db_->GetSortedWalFiles(logfiles);
+  ASSERT_GT(logfiles.size(), 0UL);
+  std::unique_ptr<LogFile> archived_log = std::move(logfiles.front());
+  ASSERT_EQ(archived_log->Type(), kArchivedLogFile);
+  ASSERT_TRUE(env_->FileExists(dbname_ + "/" + 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(dbname_ + "/" + archived_log->PathName()));
+}
+
 } //namespace rocksdb
 
 int main(int argc, char** argv) {

db/filename.cc

@@ -143,7 +143,8 @@ std::string IdentityFileName(const std::string& dbname) {
 //    Disregards / at the beginning
 bool ParseFileName(const std::string& fname,
                    uint64_t* number,
-                   FileType* type) {
+                   FileType* type,
+                   WalFileType* log_type) {
   Slice rest(fname);
   if (fname.length() > 1 && fname[0] == '/') {
     rest.remove_prefix(1);
@@ -194,6 +195,17 @@ bool ParseFileName(const std::string& fname,
   } else {
     // Avoid strtoull() to keep filename format independent of the
     // current locale
+    bool archive_dir_found = false;
+    if (rest.starts_with(ARCHIVAL_DIR)) {
+      if (rest.size() <= ARCHIVAL_DIR.size()) {
+        return false;
+      }
+      rest.remove_prefix(ARCHIVAL_DIR.size() + 1); // Add 1 to remove / also
+      if (log_type) {
+        *log_type = kArchivedLogFile;
+      }
+      archive_dir_found = true;
+    }
     uint64_t num;
     if (!ConsumeDecimalNumber(&rest, &num)) {
       return false;
@@ -201,6 +213,11 @@ bool ParseFileName(const std::string& fname,
     Slice suffix = rest;
     if (suffix == Slice(".log")) {
       *type = kLogFile;
+      if (log_type && !archive_dir_found) {
+        *log_type = kAliveLogFile;
+      }
+    } else if (archive_dir_found) {
+      return false; // Archive dir can contain only log files
     } else if (suffix == Slice(".sst")) {
       *type = kTableFile;
     } else if (suffix == Slice(".dbtmp")) {

db/filename.h

@@ -14,6 +14,7 @@
 #include <string>
 #include "rocksdb/slice.h"
 #include "rocksdb/status.h"
+#include "rocksdb/transaction_log.h"
 #include "port/port.h"
 
 namespace rocksdb {
@@ -93,7 +94,8 @@ extern std::string IdentityFileName(const std::string& dbname);
 // filename was successfully parsed, returns true.  Else return false.
 extern bool ParseFileName(const std::string& filename,
                           uint64_t* number,
-                          FileType* type);
+                          FileType* type,
+                          WalFileType* log_type = nullptr);
 
 // Make the CURRENT file point to the descriptor file with the
 // specified number.

include/rocksdb/db.h

@@ -257,10 +257,6 @@ class DB {
   // Retrieve the sorted list of all wal files with earliest file first
   virtual Status GetSortedWalFiles(VectorLogPtr& files) = 0;
 
-  // Delete wal files in files. These can be either live or archived.
-  // Returns Status::OK if all files could be deleted, otherwise Status::IOError
-  virtual Status DeleteWalFiles(const VectorLogPtr& files) = 0;
-
   // The sequence number of the most recent transaction.
   virtual SequenceNumber GetLatestSequenceNumber() = 0;
 
@@ -278,11 +274,10 @@ class DB {
   virtual Status GetUpdatesSince(SequenceNumber seq_number,
                                  unique_ptr<TransactionLogIterator>* iter) = 0;
 
-  // Delete the file name from the db directory and update the internal
-  // state to reflect that.
-  virtual Status DeleteFile(std::string name) {
-    return Status::OK();
-  }
+  // Delete the file name from the db directory and update the internal state to
+  // reflect that. Supports deletion of sst and log files only. 'name' must be
+  // path relative to the db directory. eg. 000001.sst, /archive/000003.log
+  virtual Status DeleteFile(std::string name) = 0;
 
   // Returns a list of all table files with their level, start key
   // and end key

include/rocksdb/transaction_log.h

@@ -5,6 +5,8 @@
 #include "rocksdb/status.h"
 #include "rocksdb/types.h"
 #include "rocksdb/write_batch.h"
+#include <memory>
+#include <vector>
 
 namespace rocksdb {
 

include/utilities/stackable_db.h

@@ -144,8 +144,8 @@ class StackableDB : public DB {
     return sdb_->GetSortedWalFiles(files);
   }
 
-  virtual Status DeleteWalFiles(const VectorLogPtr& files) override {
-    return sdb_->DeleteWalFiles(files);
+  virtual Status DeleteFile(std::string name) override {
+    return sdb_->DeleteFile(name);
   }
 
   virtual Status GetUpdatesSince(SequenceNumber seq_number,

utilities/ttl/db_ttl.cc

@@ -280,8 +280,8 @@ Status DBWithTTL::GetSortedWalFiles(VectorLogPtr& files) {
   return db_->GetSortedWalFiles(files);
 }
 
-Status DBWithTTL::DeleteWalFiles(const VectorLogPtr& files){
-  return db_->DeleteWalFiles(files);
+Status DBWithTTL::DeleteFile(std::string name) {
+  return db_->DeleteFile(name);
 }
 
 Status DBWithTTL::GetUpdatesSince(

utilities/ttl/db_ttl.h

@@ -78,7 +78,7 @@ class DBWithTTL : public StackableDB {
 
   virtual Status GetSortedWalFiles(VectorLogPtr& files);
 
-  virtual Status DeleteWalFiles(const VectorLogPtr& files);
+  virtual Status DeleteFile(std::string name);
 
   virtual SequenceNumber GetLatestSequenceNumber();