Remove the force mode for EnableFileDeletions API (#12337)

Summary:
There is no strong reason for user to need this mode while on the other hand, its behavior is destructive.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/12337

Reviewed By: hx235

Differential Revision: D53630393

Pulled By: jowlyzhang

fbshipit-source-id: ce94b537258102cd98f89aa4090025663664dd78

db/c.cc

@@ -1874,9 +1874,8 @@ void rocksdb_disable_file_deletions(rocksdb_t* db, char** errptr) {
   SaveError(errptr, db->rep->DisableFileDeletions());
 }
 
-void rocksdb_enable_file_deletions(rocksdb_t* db, unsigned char force,
+void rocksdb_enable_file_deletions(rocksdb_t* db, char** errptr) {
-                                   char** errptr) {
+  SaveError(errptr, db->rep->EnableFileDeletions());
-  SaveError(errptr, db->rep->EnableFileDeletions(force));
 }
 
 void rocksdb_destroy_db(const rocksdb_options_t* options, const char* name,

db/db_filesnapshot.cc

@@ -121,7 +121,7 @@ Status DBImpl::GetSortedWalFiles(VectorLogPtr& files) {
 
   // DisableFileDeletions / EnableFileDeletions not supported in read-only DB
   if (deletions_disabled.ok()) {
-    Status s2 = EnableFileDeletions(/*force=*/false);
+    Status s2 = EnableFileDeletions();
     assert(s2.ok());
     s2.PermitUncheckedError();
   } else {

db/db_impl/compacted_db_impl.h

@@ -90,7 +90,7 @@ class CompactedDBImpl : public DBImpl {
   Status DisableFileDeletions() override {
     return Status::NotSupported("Not supported in compacted db mode.");
   }
-  Status EnableFileDeletions(bool /*force*/) override {
+  Status EnableFileDeletions() override {
     return Status::NotSupported("Not supported in compacted db mode.");
   }
   Status GetLiveFiles(std::vector<std::string>& ret,

db/db_impl/db_impl.h

@@ -498,7 +498,7 @@ class DBImpl : public DB {
 
   Status DisableFileDeletions() override;
 
-  Status EnableFileDeletions(bool force) override;
+  Status EnableFileDeletions() override;
 
   virtual bool IsFileDeletionsEnabled() const;
 

db/db_impl/db_impl_files.cc

@@ -67,17 +67,14 @@ Status DBImpl::DisableFileDeletionsWithLock() {
   return Status::OK();
 }
 
-Status DBImpl::EnableFileDeletions(bool force) {
+Status DBImpl::EnableFileDeletions() {
   // Job id == 0 means that this is not our background process, but rather
   // user thread
   JobContext job_context(0);
   int saved_counter;  // initialize on all paths
   {
     InstrumentedMutexLock l(&mutex_);
-    if (force) {
+    if (disable_delete_obsolete_files_ > 0) {
-      // if force, we need to enable file deletions right away
-      disable_delete_obsolete_files_ = 0;
-    } else if (disable_delete_obsolete_files_ > 0) {
       --disable_delete_obsolete_files_;
     }
     saved_counter = disable_delete_obsolete_files_;

db/db_impl/db_impl_readonly.h

@@ -101,7 +101,7 @@ class DBImplReadOnly : public DBImpl {
     return Status::NotSupported("Not supported operation in read only mode.");
   }
 
-  Status EnableFileDeletions(bool /*force*/) override {
+  Status EnableFileDeletions() override {
     return Status::NotSupported("Not supported operation in read only mode.");
   }
   Status GetLiveFiles(std::vector<std::string>& ret,

db/db_impl/db_impl_secondary.h

@@ -189,7 +189,7 @@ class DBImplSecondary : public DBImpl {
     return Status::NotSupported("Not supported operation in secondary mode.");
   }
 
-  Status EnableFileDeletions(bool /*force*/) override {
+  Status EnableFileDeletions() override {
     return Status::NotSupported("Not supported operation in secondary mode.");
   }
 

db/db_log_iter_test.cc

@@ -236,7 +236,7 @@ TEST_F(DBTestXactLogIterator, TransactionLogIteratorCorruptedLog) {
     ASSERT_OK(test::TruncateFile(env_, logfile_path,
                                  wal_files.front()->SizeFileBytes() / 2));
 
-    ASSERT_OK(db_->EnableFileDeletions(/*force=*/false));
+    ASSERT_OK(db_->EnableFileDeletions());
 
     // Insert a new entry to a new log file
     ASSERT_OK(Put("key1025", DummyString(10)));

db/db_properties_test.cc

@@ -106,12 +106,18 @@ TEST_F(DBPropertiesTest, Empty) {
         dbfull()->GetProperty("rocksdb.is-file-deletions-enabled", &num));
     ASSERT_EQ("0", num);
 
-    ASSERT_OK(db_->EnableFileDeletions(/*force=*/false));
+    ASSERT_OK(db_->EnableFileDeletions());
     ASSERT_TRUE(
         dbfull()->GetProperty("rocksdb.is-file-deletions-enabled", &num));
     ASSERT_EQ("0", num);
 
-    ASSERT_OK(db_->EnableFileDeletions(/*force=*/true));
+    ASSERT_OK(db_->EnableFileDeletions());
+    ASSERT_TRUE(
+        dbfull()->GetProperty("rocksdb.is-file-deletions-enabled", &num));
+    ASSERT_EQ("0", num);
+    // File deletion enabled after `EnableFileDeletions` called as many times
+    // as `DisableFileDeletions`.
+    ASSERT_OK(db_->EnableFileDeletions());
     ASSERT_TRUE(
         dbfull()->GetProperty("rocksdb.is-file-deletions-enabled", &num));
     ASSERT_EQ("1", num);
@@ -1743,7 +1749,7 @@ TEST_F(DBPropertiesTest, SstFilesSize) {
   ASSERT_EQ(obsolete_sst_size, sst_size);
 
   // Let the obsolete files be deleted.
-  ASSERT_OK(db_->EnableFileDeletions(/*force=*/false));
+  ASSERT_OK(db_->EnableFileDeletions());
   ASSERT_TRUE(db_->GetIntProperty(DB::Properties::kObsoleteSstFilesSize,
                                   &obsolete_sst_size));
   ASSERT_EQ(obsolete_sst_size, 0);

db/db_test.cc

@@ -2517,7 +2517,7 @@ TEST_F(DBTest, SnapshotFiles) {
     }
 
     // release file snapshot
-    ASSERT_OK(dbfull()->EnableFileDeletions(/*force*/ false));
+    ASSERT_OK(dbfull()->EnableFileDeletions());
     // overwrite one key, this key should not appear in the snapshot
     std::vector<std::string> extras;
     for (unsigned int i = 0; i < 1; i++) {
@@ -3354,7 +3354,7 @@ class ModelDB : public DB {
 
   Status DisableFileDeletions() override { return Status::OK(); }
 
-  Status EnableFileDeletions(bool /*force*/) override { return Status::OK(); }
+  Status EnableFileDeletions() override { return Status::OK(); }
 
   Status GetLiveFiles(std::vector<std::string>&, uint64_t* /*size*/,
                       bool /*flush_memtable*/ = true) override {

db/db_test2.cc

@@ -4161,7 +4161,7 @@ TEST_F(DBTest2, LiveFilesOmitObsoleteFiles) {
     ASSERT_OK(env_->FileExists(LogFileName(dbname_, log_file->LogNumber())));
   }
 
-  ASSERT_OK(db_->EnableFileDeletions(/*force=*/false));
+  ASSERT_OK(db_->EnableFileDeletions());
   ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing();
 }
 

db/db_wal_test.cc

@@ -1195,7 +1195,7 @@ TEST_F(DBWALTest, DISABLED_FullPurgePreservesLogPendingReuse) {
     ROCKSDB_NAMESPACE::port::Thread thread([&]() {
       TEST_SYNC_POINT(
           "DBWALTest::FullPurgePreservesLogPendingReuse:PreFullPurge");
-      ASSERT_OK(db_->EnableFileDeletions(/*force=*/true));
+      ASSERT_OK(db_->EnableFileDeletions());
       TEST_SYNC_POINT(
           "DBWALTest::FullPurgePreservesLogPendingReuse:PostFullPurge");
     });

db/obsolete_files_test.cc

@@ -163,7 +163,7 @@ TEST_F(ObsoleteFilesTest, DeleteObsoleteOptionsFile) {
                                      {{"paranoid_file_checks", "true"}}));
     }
   }
-  ASSERT_OK(dbfull()->EnableFileDeletions(/*force=*/false));
+  ASSERT_OK(dbfull()->EnableFileDeletions());
 
   Close();
 

include/rocksdb/c.h

@@ -691,8 +691,8 @@ extern ROCKSDB_LIBRARY_API void rocksdb_flush_wal(rocksdb_t* db,
 extern ROCKSDB_LIBRARY_API void rocksdb_disable_file_deletions(rocksdb_t* db,
                                                                char** errptr);
 
-extern ROCKSDB_LIBRARY_API void rocksdb_enable_file_deletions(
+extern ROCKSDB_LIBRARY_API void rocksdb_enable_file_deletions(rocksdb_t* db,
-    rocksdb_t* db, unsigned char force, char** errptr);
+                                                              char** errptr);
 
 /* Management operations */
 

include/rocksdb/db.h

@@ -1669,17 +1669,10 @@ class DB {
   // critical for operations like making a backup. So the counter implementation
   // makes the file deletion disabled as long as there is one caller requesting
   // so, and only when every caller agrees to re-enable file deletion, it will
-  // be enabled. So be careful when calling this function with force = true as
+  // be enabled. Two threads can call this method concurrently without
-  // explained below.
-  // If force == true, the call to EnableFileDeletions() will guarantee that
-  // file deletions are enabled after the call, even if DisableFileDeletions()
-  // was called multiple times before.
-  // If force == false, EnableFileDeletions will only enable file deletion
-  // after it's been called at least as many times as DisableFileDeletions(),
-  // enabling the two methods to be called by two threads concurrently without
   // synchronization -- i.e., file deletions will be enabled only after both
   // threads call EnableFileDeletions()
-  virtual Status EnableFileDeletions(bool force) = 0;
+  virtual Status EnableFileDeletions() = 0;
 
   // Retrieves the creation time of the oldest file in the DB.
   // This API only works if max_open_files = -1, if it is not then

include/rocksdb/utilities/stackable_db.h

@@ -400,9 +400,7 @@ class StackableDB : public DB {
 
   Status DisableFileDeletions() override { return db_->DisableFileDeletions(); }
 
-  Status EnableFileDeletions(bool force) override {
+  Status EnableFileDeletions() override { return db_->EnableFileDeletions(); }
-    return db_->EnableFileDeletions(force);
-  }
 
   void GetLiveFilesMetaData(std::vector<LiveFileMetaData>* metadata) override {
     db_->GetLiveFilesMetaData(metadata);

java/rocksjni/rocksjni.cc

@@ -3550,10 +3550,9 @@ void Java_org_rocksdb_RocksDB_disableFileDeletions(JNIEnv* env, jclass,
  * Signature: (JZ)V
  */
 void Java_org_rocksdb_RocksDB_enableFileDeletions(JNIEnv* env, jclass,
-                                                  jlong jdb_handle,
+                                                  jlong jdb_handle) {
-                                                  jboolean jforce) {
   auto* db = reinterpret_cast<ROCKSDB_NAMESPACE::DB*>(jdb_handle);
-  ROCKSDB_NAMESPACE::Status s = db->EnableFileDeletions(jforce);
+  ROCKSDB_NAMESPACE::Status s = db->EnableFileDeletions();
   if (!s.ok()) {
     ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(env, s);
   }

java/src/main/java/org/rocksdb/RocksDB.java

@@ -4284,28 +4284,18 @@ public class RocksDB extends RocksObject {
   }
 
   /**
-   * <p>Enable deleting obsolete files.
+   * <p>EnableFileDeletions will only enable file deletion after
-   * If force == true, the call to EnableFileDeletions()
+   * it's been called at least as many times as DisableFileDeletions(),
-   * will guarantee that file deletions are enabled after
+   * enabling the two methods to be called by two threads concurrently
-   * the call, even if DisableFileDeletions() was called
+   * without synchronization
-   * multiple times before.</p>
-   *
-   * <p>If force == false, EnableFileDeletions will only
-   * enable file deletion after it's been called at least
-   * as many times as DisableFileDeletions(), enabling
-   * the two methods to be called by two threads
-   * concurrently without synchronization
    * -- i.e., file deletions will be enabled only after both
    * threads call EnableFileDeletions()</p>
    *
-   * @param force boolean value described above.
-   *
    * @throws RocksDBException thrown if operation was not performed
    *     successfully.
    */
-  public void enableFileDeletions(final boolean force)
+  public void enableFileDeletions() throws RocksDBException {
-      throws RocksDBException {
+    enableFileDeletions(nativeHandle_);
-    enableFileDeletions(nativeHandle_, force);
   }
 
   public static class LiveFiles {
@@ -5041,8 +5031,7 @@ public class RocksDB extends RocksObject {
   private static native void syncWal(final long handle) throws RocksDBException;
   private static native long getLatestSequenceNumber(final long handle);
   private static native void disableFileDeletions(long handle) throws RocksDBException;
-  private static native void enableFileDeletions(long handle, boolean force)
+  private static native void enableFileDeletions(long handle) throws RocksDBException;
-      throws RocksDBException;
   private static native String[] getLiveFiles(final long handle, final boolean flushMemtable)
       throws RocksDBException;
   private static native LogFile[] getSortedWalFiles(final long handle) throws RocksDBException;

java/src/test/java/org/rocksdb/RocksDBTest.java

@@ -1244,9 +1244,7 @@ public class RocksDBTest {
              dbFolder.getRoot().getAbsolutePath())
     ) {
       db.disableFileDeletions();
-      db.enableFileDeletions(false);
+      db.enableFileDeletions();
-      db.disableFileDeletions();
-      db.enableFileDeletions(true);
     }
   }
 

tools/ldb_cmd_test.cc

@@ -271,7 +271,7 @@ class FileChecksumTestHelper {
         break;
       }
     }
-    EXPECT_OK(db_->EnableFileDeletions(/*force=*/false));
+    EXPECT_OK(db_->EnableFileDeletions());
     return cs;
   }
 };

unreleased_history/public_api_changes/remove_force_file_deletion.md

@@ -0,0 +1 @@
+Remove the force mode for `EnableFileDeletions` API because it is unsafe with no known legitimate use.

utilities/backup/backup_engine.cc

@@ -1583,7 +1583,7 @@ IOStatus BackupEngineImpl::CreateNewBackupWithMetadata(
 
   // we copied all the files, enable file deletions
   if (disabled.ok()) {  // If we successfully disabled file deletions
-    db->EnableFileDeletions(/*force=*/false).PermitUncheckedError();
+    db->EnableFileDeletions().PermitUncheckedError();
   }
   auto backup_time = backup_env_->NowMicros() - start_backup;
 

utilities/backup/backup_engine_test.cc

@@ -86,7 +86,7 @@ class DummyDB : public StackableDB {
 
   DBOptions GetDBOptions() const override { return DBOptions(options_); }
 
-  Status EnableFileDeletions(bool /*force*/) override {
+  Status EnableFileDeletions() override {
     EXPECT_TRUE(!deletions_enabled_);
     deletions_enabled_ = true;
     return Status::OK();

utilities/blob_db/blob_db_impl.h

@@ -156,7 +156,7 @@ class BlobDBImpl : public BlobDB {
 
   Status DisableFileDeletions() override;
 
-  Status EnableFileDeletions(bool force) override;
+  Status EnableFileDeletions() override;
 
   Status GetLiveFiles(std::vector<std::string>&, uint64_t* manifest_file_size,
                       bool flush_memtable = true) override;

utilities/blob_db/blob_db_impl_filesnapshot.cc

@@ -35,9 +35,9 @@ Status BlobDBImpl::DisableFileDeletions() {
   return Status::OK();
 }
 
-Status BlobDBImpl::EnableFileDeletions(bool force) {
+Status BlobDBImpl::EnableFileDeletions() {
   // Enable base DB file deletions.
-  Status s = db_impl_->EnableFileDeletions(force);
+  Status s = db_impl_->EnableFileDeletions();
   if (!s.ok()) {
     return s;
   }
@@ -45,9 +45,7 @@ Status BlobDBImpl::EnableFileDeletions(bool force) {
   int count = 0;
   {
     MutexLock l(&delete_file_mutex_);
-    if (force) {
+    if (disable_file_deletions_ > 0) {
-      disable_file_deletions_ = 0;
-    } else if (disable_file_deletions_ > 0) {
       count = --disable_file_deletions_;
     }
     assert(count >= 0);

utilities/blob_db/blob_db_test.cc

@@ -2009,40 +2009,36 @@ TEST_F(BlobDBTest, DisableFileDeletions) {
   bdb_options.disable_background_tasks = true;
   Open(bdb_options);
   std::map<std::string, std::string> data;
-  for (bool force : {true, false}) {
+  ASSERT_OK(Put("foo", "v", &data));
-    ASSERT_OK(Put("foo", "v", &data));
+  auto blob_files = blob_db_impl()->TEST_GetBlobFiles();
-    auto blob_files = blob_db_impl()->TEST_GetBlobFiles();
+  ASSERT_EQ(1, blob_files.size());
-    ASSERT_EQ(1, blob_files.size());
+  auto blob_file = blob_files[0];
-    auto blob_file = blob_files[0];
+  ASSERT_OK(blob_db_impl()->TEST_CloseBlobFile(blob_file));
-    ASSERT_OK(blob_db_impl()->TEST_CloseBlobFile(blob_file));
+  blob_db_impl()->TEST_ObsoleteBlobFile(blob_file);
-    blob_db_impl()->TEST_ObsoleteBlobFile(blob_file);
+  ASSERT_EQ(1, blob_db_impl()->TEST_GetBlobFiles().size());
-    ASSERT_EQ(1, blob_db_impl()->TEST_GetBlobFiles().size());
+  ASSERT_EQ(1, blob_db_impl()->TEST_GetObsoleteFiles().size());
-    ASSERT_EQ(1, blob_db_impl()->TEST_GetObsoleteFiles().size());
+  // Call DisableFileDeletions twice.
-    // Call DisableFileDeletions twice.
+  ASSERT_OK(blob_db_->DisableFileDeletions());
-    ASSERT_OK(blob_db_->DisableFileDeletions());
+  ASSERT_OK(blob_db_->DisableFileDeletions());
-    ASSERT_OK(blob_db_->DisableFileDeletions());
+  // File deletions should be disabled.
-    // File deletions should be disabled.
+  blob_db_impl()->TEST_DeleteObsoleteFiles();
-    blob_db_impl()->TEST_DeleteObsoleteFiles();
+  ASSERT_EQ(1, blob_db_impl()->TEST_GetBlobFiles().size());
-    ASSERT_EQ(1, blob_db_impl()->TEST_GetBlobFiles().size());
+  ASSERT_EQ(1, blob_db_impl()->TEST_GetObsoleteFiles().size());
-    ASSERT_EQ(1, blob_db_impl()->TEST_GetObsoleteFiles().size());
+  VerifyDB(data);
-    VerifyDB(data);
+  // Enable file deletions once. File deletion will later get enabled when
-    // Enable file deletions once. If force=true, file deletion is enabled.
+  // `EnableFileDeletions` called for a second time.
-    // Otherwise it needs to enable it for a second time.
+  ASSERT_OK(blob_db_->EnableFileDeletions());
-    ASSERT_OK(blob_db_->EnableFileDeletions(force));
+  blob_db_impl()->TEST_DeleteObsoleteFiles();
-    blob_db_impl()->TEST_DeleteObsoleteFiles();
+  ASSERT_EQ(1, blob_db_impl()->TEST_GetBlobFiles().size());
-    if (!force) {
+  ASSERT_EQ(1, blob_db_impl()->TEST_GetObsoleteFiles().size());
-      ASSERT_EQ(1, blob_db_impl()->TEST_GetBlobFiles().size());
+  VerifyDB(data);
-      ASSERT_EQ(1, blob_db_impl()->TEST_GetObsoleteFiles().size());
+  // Call EnableFileDeletions a second time.
-      VerifyDB(data);
+  ASSERT_OK(blob_db_->EnableFileDeletions());
-      // Call EnableFileDeletions a second time.
+  blob_db_impl()->TEST_DeleteObsoleteFiles();
-      ASSERT_OK(blob_db_->EnableFileDeletions(/*force=*/false));
+  // File should be deleted by now.
-      blob_db_impl()->TEST_DeleteObsoleteFiles();
+  ASSERT_EQ(0, blob_db_impl()->TEST_GetBlobFiles().size());
-    }
+  ASSERT_EQ(0, blob_db_impl()->TEST_GetObsoleteFiles().size());
-    // Regardless of value of `force`, file should be deleted by now.
+  VerifyDB({});
-    ASSERT_EQ(0, blob_db_impl()->TEST_GetBlobFiles().size());
-    ASSERT_EQ(0, blob_db_impl()->TEST_GetObsoleteFiles().size());
-    VerifyDB({});
-  }
 }
 
 TEST_F(BlobDBTest, MaintainBlobFileToSstMapping) {

utilities/checkpoint/checkpoint_impl.cc

@@ -148,7 +148,7 @@ Status CheckpointImpl::CreateCheckpoint(const std::string& checkpoint_dir,
 
       // we copied all the files, enable file deletions
       if (disabled_file_deletions) {
-        Status ss = db_->EnableFileDeletions(/*force=*/false);
+        Status ss = db_->EnableFileDeletions();
         assert(ss.ok());
         ss.PermitUncheckedError();
       }
@@ -337,7 +337,7 @@ Status CheckpointImpl::ExportColumnFamily(
                             nullptr, Temperature::kUnknown);
           } /*copy_file_cb*/);
 
-      const auto enable_status = db_->EnableFileDeletions(/*force=*/false);
+      const auto enable_status = db_->EnableFileDeletions();
       if (s.ok()) {
         s = enable_status;
       }