From e541dcc8fa1a3c5c74edc0282ea5de0155b7b94f Mon Sep 17 00:00:00 2001
From: Igor Canadi <icanadi@fb.com>
Date: Wed, 6 Jan 2016 13:05:24 -0800
Subject: [PATCH] Fix issue #921

Summary:
See a bug report here: https://github.com/facebook/rocksdb/issues/921
The fix is to not check the shared/ directory if share_table_files is false. We could also check FileExists() before GetChildren(), but that will add extra latency when Env is Hdfs :(

Test Plan: added a unit test

Reviewers: rven, sdong, IslamAbdelRahman, yhchiang, anthony

Reviewed By: anthony

Subscribers: dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D52593
---
 utilities/backupable/backupable_db.cc      | 42 +++++++++++-----------
 utilities/backupable/backupable_db_test.cc | 10 ++++++
 2 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/utilities/backupable/backupable_db.cc b/utilities/backupable/backupable_db.cc
index b4964950a..bbaf75b98 100644
--- a/utilities/backupable/backupable_db.cc
+++ b/utilities/backupable/backupable_db.cc
@@ -1344,27 +1344,29 @@ Status BackupEngineImpl::GarbageCollect() {
   assert(!read_only_);
   Log(options_.info_log, "Starting garbage collection");
 
-  // delete obsolete shared files
-  std::vector<std::string> shared_children;
-  {
-    auto s = backup_env_->GetChildren(GetAbsolutePath(GetSharedFileRel()),
-                                      &shared_children);
-    if (!s.ok()) {
-      return s;
+  if (options_.share_table_files) {
+    // delete obsolete shared files
+    std::vector<std::string> shared_children;
+    {
+      auto s = backup_env_->GetChildren(GetAbsolutePath(GetSharedFileRel()),
+                                        &shared_children);
+      if (!s.ok()) {
+        return s;
+      }
     }
-  }
-  for (auto& child : shared_children) {
-    std::string rel_fname = GetSharedFileRel(child);
-    auto child_itr = backuped_file_infos_.find(rel_fname);
-    // if it's not refcounted, delete it
-    if (child_itr == backuped_file_infos_.end() ||
-        child_itr->second->refs == 0) {
-      // this might be a directory, but DeleteFile will just fail in that
-      // case, so we're good
-      Status s = backup_env_->DeleteFile(GetAbsolutePath(rel_fname));
-      Log(options_.info_log, "Deleting %s -- %s", rel_fname.c_str(),
-          s.ToString().c_str());
-      backuped_file_infos_.erase(rel_fname);
+    for (auto& child : shared_children) {
+      std::string rel_fname = GetSharedFileRel(child);
+      auto child_itr = backuped_file_infos_.find(rel_fname);
+      // if it's not refcounted, delete it
+      if (child_itr == backuped_file_infos_.end() ||
+          child_itr->second->refs == 0) {
+        // this might be a directory, but DeleteFile will just fail in that
+        // case, so we're good
+        Status s = backup_env_->DeleteFile(GetAbsolutePath(rel_fname));
+        Log(options_.info_log, "Deleting %s -- %s", rel_fname.c_str(),
+            s.ToString().c_str());
+        backuped_file_infos_.erase(rel_fname);
+      }
     }
   }
 
diff --git a/utilities/backupable/backupable_db_test.cc b/utilities/backupable/backupable_db_test.cc
index b4f56bf7a..68b5f4286 100644
--- a/utilities/backupable/backupable_db_test.cc
+++ b/utilities/backupable/backupable_db_test.cc
@@ -1313,6 +1313,16 @@ TEST_F(BackupableDBTest, EnvFailures) {
     delete backup_engine;
   }
 }
+
+// see https://github.com/facebook/rocksdb/issues/921
+TEST_F(BackupableDBTest, Issue921Test) {
+  BackupEngine* backup_engine;
+  backupable_options_->share_table_files = false;
+  backupable_options_->backup_dir += "/new_dir";
+
+  ASSERT_OK(BackupEngine::Open(env_, *backupable_options_, &backup_engine));
+}
+
 }  // anon namespace
 
 } //  namespace rocksdb