Remove fail_if_options_file_error DB option (#13504)

Summary:
The fail_if_options_file_error has been deprecated for more than a year. This PR removes it from the code base. https://github.com/facebook/rocksdb/issues/12056 fixed a bug that was blocking the option from removal. https://github.com/facebook/rocksdb/issues/12249 marked it as deprecated.

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

Reviewed By: hx235

Differential Revision: D72194063

Pulled By: anand1976

fbshipit-source-id: 0aa7cf56e60c48c7e7654743d3e64922ce65225d

db/column_family_test.cc

@@ -72,7 +72,6 @@ class ColumnFamilyTestBase : public testing::Test {
     env_->skip_fsync_ = true;
     dbname_ = test::PerThreadDBPath("column_family_test");
     db_options_.create_if_missing = true;
-    db_options_.fail_if_options_file_error = true;
     db_options_.env = env_;
   }
 

db/db_filesnapshot.cc

@@ -75,11 +75,9 @@ Status DBImpl::GetLiveFiles(std::vector<std::string>& ret,
 
   ret.emplace_back(CurrentFileName(""));
   ret.emplace_back(DescriptorFileName("", versions_->manifest_file_number()));
-  // The OPTIONS file number is zero in read-write mode when OPTIONS file
-  // writing failed and the DB was configured with
-  // `fail_if_options_file_error == false`. In read-only mode the OPTIONS file
-  // number is zero when no OPTIONS file exist at all. In those cases we do not
-  // record any OPTIONS file in the live file list.
+  // In read-only mode the OPTIONS file number is zero when no OPTIONS file
+  // exist at all. In this cases we do not record any OPTIONS file in the live
+  // file list.
   if (versions_->options_file_number() != 0) {
     ret.emplace_back(OptionsFileName("", versions_->options_file_number()));
   }
@@ -369,11 +367,9 @@ Status DBImpl::GetLiveFilesStorageInfo(
     }
   }
 
-  // The OPTIONS file number is zero in read-write mode when OPTIONS file
-  // writing failed and the DB was configured with
-  // `fail_if_options_file_error == false`. In read-only mode the OPTIONS file
-  // number is zero when no OPTIONS file exist at all. In those cases we do not
-  // record any OPTIONS file in the live file list.
+  // In read-only mode the OPTIONS file number is zero when no OPTIONS file
+  // exist at all. In this cases we do not record any OPTIONS file in the live
+  // file list.
   if (options_number != 0) {
     results.emplace_back();
     LiveFileStorageInfo& info = results.back();

db/db_impl/db_impl.cc

@@ -1470,14 +1470,9 @@ Status DBImpl::SetDBOptions(
     ROCKS_LOG_INFO(immutable_db_options_.info_log, "SetDBOptions() succeeded");
     new_options.Dump(immutable_db_options_.info_log.get());
     if (!persist_options_status.ok()) {
-      if (immutable_db_options_.fail_if_options_file_error) {
-        s = Status::IOError(
-            "SetDBOptions() succeeded, but unable to persist options",
-            persist_options_status.ToString());
-      }
-      ROCKS_LOG_WARN(immutable_db_options_.info_log,
-                     "Unable to persist options in SetDBOptions() -- %s",
-                     persist_options_status.ToString().c_str());
+      s = Status::IOError(
+          "SetDBOptions() succeeded, but unable to persist options",
+          persist_options_status.ToString());
     }
   } else {
     ROCKS_LOG_WARN(immutable_db_options_.info_log, "SetDBOptions failed");
@@ -5445,12 +5440,7 @@ Status DBImpl::WriteOptionsFile(const WriteOptions& write_options,
   if (!s.ok()) {
     ROCKS_LOG_WARN(immutable_db_options_.info_log,
                    "Unnable to persist options -- %s", s.ToString().c_str());
-    if (immutable_db_options_.fail_if_options_file_error) {
-      s = Status::IOError("Unable to persist options.", s.ToString().c_str());
-    } else {
-      // Ignore error
-      s = Status::OK();
-    }
+    s = Status::IOError("Unable to persist options.", s.ToString().c_str());
   }
 
   // Restore lock if appropriate

db/db_options_test.cc

@@ -322,31 +322,26 @@ TEST_F(DBOptionsTest, SetWithCustomMemTableFactory) {
   }
   Options options;
   options.create_if_missing = true;
-  // Try with fail_if_options_file_error=false/true to update the options
-  for (bool on_error : {false, true}) {
-    options.fail_if_options_file_error = on_error;
-    options.env = env_;
-    options.disable_auto_compactions = false;
+  options.env = env_;
+  options.disable_auto_compactions = false;
 
-    options.memtable_factory.reset(new DummySkipListFactory());
-    Reopen(options);
+  options.memtable_factory.reset(new DummySkipListFactory());
+  Reopen(options);
 
-    ColumnFamilyHandle* cfh = dbfull()->DefaultColumnFamily();
-    ASSERT_OK(
-        dbfull()->SetOptions(cfh, {{"disable_auto_compactions", "true"}}));
-    ColumnFamilyDescriptor cfd;
-    ASSERT_OK(cfh->GetDescriptor(&cfd));
-    ASSERT_STREQ(cfd.options.memtable_factory->Name(),
-                 DummySkipListFactory::kClassName());
-    ColumnFamilyHandle* test = nullptr;
-    ASSERT_OK(dbfull()->CreateColumnFamily(options, "test", &test));
-    ASSERT_OK(test->GetDescriptor(&cfd));
-    ASSERT_STREQ(cfd.options.memtable_factory->Name(),
-                 DummySkipListFactory::kClassName());
+  ColumnFamilyHandle* cfh = dbfull()->DefaultColumnFamily();
+  ASSERT_OK(dbfull()->SetOptions(cfh, {{"disable_auto_compactions", "true"}}));
+  ColumnFamilyDescriptor cfd;
+  ASSERT_OK(cfh->GetDescriptor(&cfd));
+  ASSERT_STREQ(cfd.options.memtable_factory->Name(),
+               DummySkipListFactory::kClassName());
+  ColumnFamilyHandle* test = nullptr;
+  ASSERT_OK(dbfull()->CreateColumnFamily(options, "test", &test));
+  ASSERT_OK(test->GetDescriptor(&cfd));
+  ASSERT_STREQ(cfd.options.memtable_factory->Name(),
+               DummySkipListFactory::kClassName());
 
-    ASSERT_OK(dbfull()->DropColumnFamily(test));
-    delete test;
-  }
+  ASSERT_OK(dbfull()->DropColumnFamily(test));
+  delete test;
 }
 
 TEST_F(DBOptionsTest, SetBytesPerSync) {

db/db_test_util.cc

@@ -592,7 +592,6 @@ Options DBTestBase::GetOptions(
       options_override.level_compaction_dynamic_level_bytes;
   options.env = env_;
   options.create_if_missing = true;
-  options.fail_if_options_file_error = true;
   return options;
 }
 

db_stress_tool/db_stress_common.h

@@ -320,7 +320,6 @@ DECLARE_int32(approximate_size_one_in);
 DECLARE_bool(best_efforts_recovery);
 DECLARE_bool(skip_verifydb);
 DECLARE_bool(paranoid_file_checks);
-DECLARE_bool(fail_if_options_file_error);
 DECLARE_uint64(batch_protection_bytes_per_key);
 DECLARE_uint32(memtable_protection_bytes_per_key);
 DECLARE_uint32(block_protection_bytes_per_key);

db_stress_tool/db_stress_gflags.cc

@@ -1084,10 +1084,6 @@ DEFINE_bool(paranoid_file_checks, true,
             "After writing every SST file, reopen it and read all the keys "
             "and validate checksums");
 
-DEFINE_bool(fail_if_options_file_error, false,
-            "Fail operations that fail to detect or properly persist options "
-            "file.");
-
 DEFINE_uint64(batch_protection_bytes_per_key, 0,
               "If nonzero, enables integrity protection in `WriteBatch` at the "
               "specified number of bytes per key. Currently the only supported "

db_stress_tool/db_stress_test_base.cc

@@ -3376,8 +3376,6 @@ void StressTest::PrintEnv() const {
           FLAGS_sync_fault_injection);
   fprintf(stdout, "Best efforts recovery     : %d\n",
           static_cast<int>(FLAGS_best_efforts_recovery));
-  fprintf(stdout, "Fail if OPTIONS file error: %d\n",
-          static_cast<int>(FLAGS_fail_if_options_file_error));
   fprintf(stdout, "User timestamp size bytes : %d\n",
           static_cast<int>(FLAGS_user_timestamp_size));
   fprintf(stdout, "Persist user defined timestamps : %d\n",
@@ -4255,7 +4253,6 @@ void InitializeOptionsFromFlags(
 
   options.best_efforts_recovery = FLAGS_best_efforts_recovery;
   options.paranoid_file_checks = FLAGS_paranoid_file_checks;
-  options.fail_if_options_file_error = FLAGS_fail_if_options_file_error;
 
   if (FLAGS_user_timestamp_size > 0) {
     CheckAndSetOptionsForUserTimestamp(options);

include/rocksdb/options.h

@@ -1297,14 +1297,6 @@ struct DBOptions {
   // currently.
   WalFilter* wal_filter = nullptr;
 
-  // DEPRECATED: This option might be removed in a future release.
-  //
-  // If true, then DB::Open, CreateColumnFamily, DropColumnFamily, and
-  // SetOptions will fail if options file is not properly persisted.
-  //
-  // DEFAULT: true
-  bool fail_if_options_file_error = true;
-
   // If true, then print malloc stats together with rocksdb.stats
   // when printing to LOG.
   // DEFAULT: false

java/rocksjni/options.cc

@@ -2055,29 +2055,6 @@ void Java_org_rocksdb_Options_setWalFilter(JNIEnv*, jclass, jlong jhandle,
   opt->wal_filter = wal_filter;
 }
 
-/*
- * Class:     org_rocksdb_Options
- * Method:    setFailIfOptionsFileError
- * Signature: (JZ)V
- */
-void Java_org_rocksdb_Options_setFailIfOptionsFileError(
-    JNIEnv*, jclass, jlong jhandle, jboolean jfail_if_options_file_error) {
-  auto* opt = reinterpret_cast<ROCKSDB_NAMESPACE::Options*>(jhandle);
-  opt->fail_if_options_file_error =
-      static_cast<bool>(jfail_if_options_file_error);
-}
-
-/*
- * Class:     org_rocksdb_Options
- * Method:    failIfOptionsFileError
- * Signature: (J)Z
- */
-jboolean Java_org_rocksdb_Options_failIfOptionsFileError(JNIEnv*, jclass,
-                                                         jlong jhandle) {
-  auto* opt = reinterpret_cast<ROCKSDB_NAMESPACE::Options*>(jhandle);
-  return static_cast<jboolean>(opt->fail_if_options_file_error);
-}
-
 /*
  * Class:     org_rocksdb_Options
  * Method:    setDumpMallocStats
@@ -7479,29 +7456,6 @@ void Java_org_rocksdb_DBOptions_setWalFilter(JNIEnv*, jclass, jlong jhandle,
   opt->wal_filter = wal_filter;
 }
 
-/*
- * Class:     org_rocksdb_DBOptions
- * Method:    setFailIfOptionsFileError
- * Signature: (JZ)V
- */
-void Java_org_rocksdb_DBOptions_setFailIfOptionsFileError(
-    JNIEnv*, jclass, jlong jhandle, jboolean jfail_if_options_file_error) {
-  auto* opt = reinterpret_cast<ROCKSDB_NAMESPACE::DBOptions*>(jhandle);
-  opt->fail_if_options_file_error =
-      static_cast<bool>(jfail_if_options_file_error);
-}
-
-/*
- * Class:     org_rocksdb_DBOptions
- * Method:    failIfOptionsFileError
- * Signature: (J)Z
- */
-jboolean Java_org_rocksdb_DBOptions_failIfOptionsFileError(JNIEnv*, jclass,
-                                                           jlong jhandle) {
-  auto* opt = reinterpret_cast<ROCKSDB_NAMESPACE::DBOptions*>(jhandle);
-  return static_cast<jboolean>(opt->fail_if_options_file_error);
-}
-
 /*
  * Class:     org_rocksdb_DBOptions
  * Method:    setDumpMallocStats

java/src/test/java/org/rocksdb/DBOptionsTest.java

@@ -656,15 +656,6 @@ public class DBOptionsTest {
     }
   }
 
-  @Test
-  public void failIfOptionsFileError() {
-    try (final DBOptions opt = new DBOptions()) {
-      final boolean boolValue = rand.nextBoolean();
-      opt.setFailIfOptionsFileError(boolValue);
-      assertThat(opt.failIfOptionsFileError()).isEqualTo(boolValue);
-    }
-  }
-
   @Test
   public void dumpMallocStats() {
     try (final DBOptions opt = new DBOptions()) {

java/src/test/java/org/rocksdb/OptionsTest.java

@@ -902,15 +902,6 @@ public class OptionsTest {
     }
   }
 
-  @Test
-  public void failIfOptionsFileError() {
-    try (final Options opt = new Options()) {
-      final boolean boolValue = rand.nextBoolean();
-      opt.setFailIfOptionsFileError(boolValue);
-      assertThat(opt.failIfOptionsFileError()).isEqualTo(boolValue);
-    }
-  }
-
   @Test
   public void dumpMallocStats() {
     try (final Options opt = new Options()) {

options/db_options.cc

@@ -319,8 +319,7 @@ static std::unordered_map<std::string, OptionTypeInfo>
           OptionType::kBoolean, OptionVerificationType::kNormal,
           OptionTypeFlags::kNone}},
         {"fail_if_options_file_error",
-         {offsetof(struct ImmutableDBOptions, fail_if_options_file_error),
-          OptionType::kBoolean, OptionVerificationType::kNormal,
+         {0, OptionType::kBoolean, OptionVerificationType::kDeprecated,
           OptionTypeFlags::kNone}},
         {"enable_pipelined_write",
          {offsetof(struct ImmutableDBOptions, enable_pipelined_write),
@@ -772,7 +771,6 @@ ImmutableDBOptions::ImmutableDBOptions(const DBOptions& options)
       allow_2pc(options.allow_2pc),
       row_cache(options.row_cache),
       wal_filter(options.wal_filter),
-      fail_if_options_file_error(options.fail_if_options_file_error),
       dump_malloc_stats(options.dump_malloc_stats),
       avoid_flush_during_recovery(options.avoid_flush_during_recovery),
       allow_ingest_behind(options.allow_ingest_behind),

options/db_options.h

@@ -77,7 +77,6 @@ struct ImmutableDBOptions {
   bool allow_2pc;
   std::shared_ptr<Cache> row_cache;
   WalFilter* wal_filter;
-  bool fail_if_options_file_error;
   bool dump_malloc_stats;
   bool avoid_flush_during_recovery;
   bool allow_ingest_behind;

options/options_helper.cc

@@ -153,8 +153,6 @@ void BuildDBOptions(const ImmutableDBOptions& immutable_db_options,
   options.allow_2pc = immutable_db_options.allow_2pc;
   options.row_cache = immutable_db_options.row_cache;
   options.wal_filter = immutable_db_options.wal_filter;
-  options.fail_if_options_file_error =
-      immutable_db_options.fail_if_options_file_error;
   options.dump_malloc_stats = immutable_db_options.dump_malloc_stats;
   options.avoid_flush_during_recovery =
       immutable_db_options.avoid_flush_during_recovery;

options/options_settable_test.cc

@@ -433,7 +433,6 @@ TEST_F(OptionsSettableTest, DBOptionsAllFieldsSettable) {
                              "use_direct_io_for_flush_and_compaction=false;"
                              "max_log_file_size=4607;"
                              "advise_random_on_open=true;"
-                             "fail_if_options_file_error=false;"
                              "enable_pipelined_write=false;"
                              "unordered_write=false;"
                              "allow_concurrent_memtable_write=true;"

tools/db_crashtest.py

@@ -91,7 +91,6 @@ default_params = {
     # (see below `finalize_and_sanitize`).
     "inplace_update_support": random.choice([0] * 9 + [1]),
     "expected_values_dir": lambda: setup_expected_values_dir(),
-    "fail_if_options_file_error": lambda: random.randint(0, 1),
     "flush_one_in": lambda: random.choice([1000, 1000000]),
     "manual_wal_flush_one_in": lambda: random.choice([0, 1000]),
     "file_checksum_impl": lambda: random.choice(["none", "crc32c", "xxh64", "big"]),

unreleased_history/public_api_changes/remove_fail_if_options_file_error.md

@@ -0,0 +1 @@
+The fail_if_options_file_error option in DBOptions has been removed. The behavior now is to always return failure in any API that fails to persist the OPTIONS file.

utilities/checkpoint/checkpoint_test.cc

@@ -847,54 +847,6 @@ TEST_P(CheckpointTestWithWalParams, CheckpointWithUnsyncedDataDropped) {
   db_ = nullptr;
 }
 
-TEST_F(CheckpointTest, CheckpointOptionsFileFailedToPersist) {
-  // Regression test for a bug where checkpoint failed on a DB where persisting
-  // OPTIONS file failed and the DB was opened with
-  // `fail_if_options_file_error == false`.
-  Options options = CurrentOptions();
-  options.fail_if_options_file_error = false;
-  auto fault_fs = std::make_shared<FaultInjectionTestFS>(FileSystem::Default());
-
-  // Setup `FaultInjectionTestFS` and `SyncPoint` callbacks to fail one
-  // operation when inside the OPTIONS file persisting code.
-  std::unique_ptr<Env> fault_fs_env(NewCompositeEnv(fault_fs));
-  fault_fs->SetThreadLocalErrorContext(
-      FaultInjectionIOType::kWrite, 7 /* seed*/, 1 /* one_in */,
-      false /* retryable */, false /* has_data_loss*/);
-  SyncPoint::GetInstance()->SetCallBack(
-      "PersistRocksDBOptions:start", [fault_fs](void* /* arg */) {
-        fault_fs->EnableThreadLocalErrorInjection(
-            FaultInjectionIOType::kMetadataWrite);
-      });
-  SyncPoint::GetInstance()->SetCallBack(
-      "FaultInjectionTestFS::InjectMetadataWriteError:Injected",
-      [fault_fs](void* /* arg */) {
-        fault_fs->DisableThreadLocalErrorInjection(
-            FaultInjectionIOType::kMetadataWrite);
-      });
-  options.env = fault_fs_env.get();
-  SyncPoint::GetInstance()->EnableProcessing();
-
-  Reopen(options);
-  ASSERT_OK(Put("key1", "val1"));
-  Checkpoint* checkpoint;
-  ASSERT_OK(Checkpoint::Create(db_, &checkpoint));
-  ASSERT_OK(checkpoint->CreateCheckpoint(snapshot_name_));
-  delete checkpoint;
-
-  // Make sure it's usable.
-  options.env = env_;
-  DB* snapshot_db;
-  ASSERT_OK(DB::Open(options, snapshot_name_, &snapshot_db));
-  ReadOptions read_opts;
-  std::string get_result;
-  ASSERT_OK(snapshot_db->Get(read_opts, "key1", &get_result));
-  ASSERT_EQ("val1", get_result);
-  delete snapshot_db;
-  delete db_;
-  db_ = nullptr;
-}
-
 TEST_F(CheckpointTest, CheckpointReadOnlyDB) {
   ASSERT_OK(Put("foo", "foo_value"));
   ASSERT_OK(Flush());

utilities/ttl/ttl_test.cc

@@ -617,7 +617,6 @@ TEST_F(TtlTest, UnregisteredMergeOperator) {
    public:
     const char* Name() const override { return "UnregisteredMergeOperator"; }
   };
-  options_.fail_if_options_file_error = true;
   options_.merge_operator = std::make_shared<UnregisteredMergeOperator>();
   OpenTtl();
   CloseTtl();