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

@@ -1344,27 +1344,29 @@ Status BackupEngineImpl::GarbageCollect() {
   assert(!read_only_);
   Log(options_.info_log, "Starting garbage collection");
 
-  // delete obsolete shared files
+  if (options_.share_table_files) {
-  std::vector<std::string> shared_children;
+    // delete obsolete shared files
-  {
+    std::vector<std::string> shared_children;
-    auto s = backup_env_->GetChildren(GetAbsolutePath(GetSharedFileRel()),
+    {
-                                      &shared_children);
+      auto s = backup_env_->GetChildren(GetAbsolutePath(GetSharedFileRel()),
-    if (!s.ok()) {
+                                        &shared_children);
-      return s;
+      if (!s.ok()) {
+        return s;
+      }
     }
-  }
+    for (auto& child : shared_children) {
-  for (auto& child : shared_children) {
+      std::string rel_fname = GetSharedFileRel(child);
-    std::string rel_fname = GetSharedFileRel(child);
+      auto child_itr = backuped_file_infos_.find(rel_fname);
-    auto child_itr = backuped_file_infos_.find(rel_fname);
+      // if it's not refcounted, delete it
-    // if it's not refcounted, delete it
+      if (child_itr == backuped_file_infos_.end() ||
-    if (child_itr == backuped_file_infos_.end() ||
+          child_itr->second->refs == 0) {
-        child_itr->second->refs == 0) {
+        // this might be a directory, but DeleteFile will just fail in that
-      // this might be a directory, but DeleteFile will just fail in that
+        // case, so we're good
-      // case, so we're good
+        Status s = backup_env_->DeleteFile(GetAbsolutePath(rel_fname));
-      Status s = backup_env_->DeleteFile(GetAbsolutePath(rel_fname));
+        Log(options_.info_log, "Deleting %s -- %s", rel_fname.c_str(),
-      Log(options_.info_log, "Deleting %s -- %s", rel_fname.c_str(),
+            s.ToString().c_str());
-          s.ToString().c_str());
+        backuped_file_infos_.erase(rel_fname);
-      backuped_file_infos_.erase(rel_fname);
+      }
     }
   }
 

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