Skip to content

Commit f7764cb

Browse files
anand1976facebook-github-bot
anand1976
authored and
facebook-github-bot
committed
Remove fail_if_options_file_error DB option (facebook#13504)
Summary: The fail_if_options_file_error has been deprecated for more than a year. This PR removes it from the code base. facebook#12056 fixed a bug that was blocking the option from removal. facebook#12249 marked it as deprecated. Pull Request resolved: facebook#13504 Reviewed By: hx235 Differential Revision: D72194063 Pulled By: anand1976 fbshipit-source-id: 0aa7cf56e60c48c7e7654743d3e64922ce65225d
1 parent 6d80263 commit f7764cb

File tree

20 files changed

+32
-188
lines changed

20 files changed

+32
-188
lines changed

‎db/column_family_test.cc

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@ class ColumnFamilyTestBase : public testing::Test {
7272
env_->skip_fsync_ = true;
7373
dbname_ = test::PerThreadDBPath("column_family_test");
7474
db_options_.create_if_missing = true;
75-
db_options_.fail_if_options_file_error = true;
7675
db_options_.env = env_;
7776
}
7877

‎db/db_filesnapshot.cc

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -75,11 +75,9 @@ Status DBImpl::GetLiveFiles(std::vector<std::string>& ret,
7575

7676
ret.emplace_back(CurrentFileName(""));
7777
ret.emplace_back(DescriptorFileName("", versions_->manifest_file_number()));
78-
// The OPTIONS file number is zero in read-write mode when OPTIONS file
79-
// writing failed and the DB was configured with
80-
// `fail_if_options_file_error == false`. In read-only mode the OPTIONS file
81-
// number is zero when no OPTIONS file exist at all. In those cases we do not
82-
// record any OPTIONS file in the live file list.
78+
// In read-only mode the OPTIONS file number is zero when no OPTIONS file
79+
// exist at all. In this cases we do not record any OPTIONS file in the live
80+
// file list.
8381
if (versions_->options_file_number() != 0) {
8482
ret.emplace_back(OptionsFileName("", versions_->options_file_number()));
8583
}
@@ -369,11 +367,9 @@ Status DBImpl::GetLiveFilesStorageInfo(
369367
}
370368
}
371369

372-
// The OPTIONS file number is zero in read-write mode when OPTIONS file
373-
// writing failed and the DB was configured with
374-
// `fail_if_options_file_error == false`. In read-only mode the OPTIONS file
375-
// number is zero when no OPTIONS file exist at all. In those cases we do not
376-
// record any OPTIONS file in the live file list.
370+
// In read-only mode the OPTIONS file number is zero when no OPTIONS file
371+
// exist at all. In this cases we do not record any OPTIONS file in the live
372+
// file list.
377373
if (options_number != 0) {
378374
results.emplace_back();
379375
LiveFileStorageInfo& info = results.back();

‎db/db_impl/db_impl.cc

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1470,14 +1470,9 @@ Status DBImpl::SetDBOptions(
14701470
ROCKS_LOG_INFO(immutable_db_options_.info_log, "SetDBOptions() succeeded");
14711471
new_options.Dump(immutable_db_options_.info_log.get());
14721472
if (!persist_options_status.ok()) {
1473-
if (immutable_db_options_.fail_if_options_file_error) {
1474-
s = Status::IOError(
1475-
"SetDBOptions() succeeded, but unable to persist options",
1476-
persist_options_status.ToString());
1477-
}
1478-
ROCKS_LOG_WARN(immutable_db_options_.info_log,
1479-
"Unable to persist options in SetDBOptions() -- %s",
1480-
persist_options_status.ToString().c_str());
1473+
s = Status::IOError(
1474+
"SetDBOptions() succeeded, but unable to persist options",
1475+
persist_options_status.ToString());
14811476
}
14821477
} else {
14831478
ROCKS_LOG_WARN(immutable_db_options_.info_log, "SetDBOptions failed");
@@ -5445,12 +5440,7 @@ Status DBImpl::WriteOptionsFile(const WriteOptions& write_options,
54455440
if (!s.ok()) {
54465441
ROCKS_LOG_WARN(immutable_db_options_.info_log,
54475442
"Unnable to persist options -- %s", s.ToString().c_str());
5448-
if (immutable_db_options_.fail_if_options_file_error) {
5449-
s = Status::IOError("Unable to persist options.", s.ToString().c_str());
5450-
} else {
5451-
// Ignore error
5452-
s = Status::OK();
5453-
}
5443+
s = Status::IOError("Unable to persist options.", s.ToString().c_str());
54545444
}
54555445

54565446
// Restore lock if appropriate

‎db/db_options_test.cc

Lines changed: 20 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -322,31 +322,26 @@ TEST_F(DBOptionsTest, SetWithCustomMemTableFactory) {
322322
}
323323
Options options;
324324
options.create_if_missing = true;
325-
// Try with fail_if_options_file_error=false/true to update the options
326-
for (bool on_error : {false, true}) {
327-
options.fail_if_options_file_error = on_error;
328-
options.env = env_;
329-
options.disable_auto_compactions = false;
330-
331-
options.memtable_factory.reset(new DummySkipListFactory());
332-
Reopen(options);
333-
334-
ColumnFamilyHandle* cfh = dbfull()->DefaultColumnFamily();
335-
ASSERT_OK(
336-
dbfull()->SetOptions(cfh, {{"disable_auto_compactions", "true"}}));
337-
ColumnFamilyDescriptor cfd;
338-
ASSERT_OK(cfh->GetDescriptor(&cfd));
339-
ASSERT_STREQ(cfd.options.memtable_factory->Name(),
340-
DummySkipListFactory::kClassName());
341-
ColumnFamilyHandle* test = nullptr;
342-
ASSERT_OK(dbfull()->CreateColumnFamily(options, "test", &test));
343-
ASSERT_OK(test->GetDescriptor(&cfd));
344-
ASSERT_STREQ(cfd.options.memtable_factory->Name(),
345-
DummySkipListFactory::kClassName());
346-
347-
ASSERT_OK(dbfull()->DropColumnFamily(test));
348-
delete test;
349-
}
325+
options.env = env_;
326+
options.disable_auto_compactions = false;
327+
328+
options.memtable_factory.reset(new DummySkipListFactory());
329+
Reopen(options);
330+
331+
ColumnFamilyHandle* cfh = dbfull()->DefaultColumnFamily();
332+
ASSERT_OK(dbfull()->SetOptions(cfh, {{"disable_auto_compactions", "true"}}));
333+
ColumnFamilyDescriptor cfd;
334+
ASSERT_OK(cfh->GetDescriptor(&cfd));
335+
ASSERT_STREQ(cfd.options.memtable_factory->Name(),
336+
DummySkipListFactory::kClassName());
337+
ColumnFamilyHandle* test = nullptr;
338+
ASSERT_OK(dbfull()->CreateColumnFamily(options, "test", &test));
339+
ASSERT_OK(test->GetDescriptor(&cfd));
340+
ASSERT_STREQ(cfd.options.memtable_factory->Name(),
341+
DummySkipListFactory::kClassName());
342+
343+
ASSERT_OK(dbfull()->DropColumnFamily(test));
344+
delete test;
350345
}
351346

352347
TEST_F(DBOptionsTest, SetBytesPerSync) {

‎db/db_test_util.cc

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -592,7 +592,6 @@ Options DBTestBase::GetOptions(
592592
options_override.level_compaction_dynamic_level_bytes;
593593
options.env = env_;
594594
options.create_if_missing = true;
595-
options.fail_if_options_file_error = true;
596595
return options;
597596
}
598597

‎db_stress_tool/db_stress_common.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,6 @@ DECLARE_int32(approximate_size_one_in);
320320
DECLARE_bool(best_efforts_recovery);
321321
DECLARE_bool(skip_verifydb);
322322
DECLARE_bool(paranoid_file_checks);
323-
DECLARE_bool(fail_if_options_file_error);
324323
DECLARE_uint64(batch_protection_bytes_per_key);
325324
DECLARE_uint32(memtable_protection_bytes_per_key);
326325
DECLARE_uint32(block_protection_bytes_per_key);

‎db_stress_tool/db_stress_gflags.cc

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1084,10 +1084,6 @@ DEFINE_bool(paranoid_file_checks, true,
10841084
"After writing every SST file, reopen it and read all the keys "
10851085
"and validate checksums");
10861086

1087-
DEFINE_bool(fail_if_options_file_error, false,
1088-
"Fail operations that fail to detect or properly persist options "
1089-
"file.");
1090-
10911087
DEFINE_uint64(batch_protection_bytes_per_key, 0,
10921088
"If nonzero, enables integrity protection in `WriteBatch` at the "
10931089
"specified number of bytes per key. Currently the only supported "

‎db_stress_tool/db_stress_test_base.cc

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3376,8 +3376,6 @@ void StressTest::PrintEnv() const {
33763376
FLAGS_sync_fault_injection);
33773377
fprintf(stdout, "Best efforts recovery : %d\n",
33783378
static_cast<int>(FLAGS_best_efforts_recovery));
3379-
fprintf(stdout, "Fail if OPTIONS file error: %d\n",
3380-
static_cast<int>(FLAGS_fail_if_options_file_error));
33813379
fprintf(stdout, "User timestamp size bytes : %d\n",
33823380
static_cast<int>(FLAGS_user_timestamp_size));
33833381
fprintf(stdout, "Persist user defined timestamps : %d\n",
@@ -4255,7 +4253,6 @@ void InitializeOptionsFromFlags(
42554253

42564254
options.best_efforts_recovery = FLAGS_best_efforts_recovery;
42574255
options.paranoid_file_checks = FLAGS_paranoid_file_checks;
4258-
options.fail_if_options_file_error = FLAGS_fail_if_options_file_error;
42594256

42604257
if (FLAGS_user_timestamp_size > 0) {
42614258
CheckAndSetOptionsForUserTimestamp(options);

‎include/rocksdb/options.h

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1297,14 +1297,6 @@ struct DBOptions {
12971297
// currently.
12981298
WalFilter* wal_filter = nullptr;
12991299

1300-
// DEPRECATED: This option might be removed in a future release.
1301-
//
1302-
// If true, then DB::Open, CreateColumnFamily, DropColumnFamily, and
1303-
// SetOptions will fail if options file is not properly persisted.
1304-
//
1305-
// DEFAULT: true
1306-
bool fail_if_options_file_error = true;
1307-
13081300
// If true, then print malloc stats together with rocksdb.stats
13091301
// when printing to LOG.
13101302
// DEFAULT: false

‎java/rocksjni/options.cc

Lines changed: 0 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -2055,29 +2055,6 @@ void Java_org_rocksdb_Options_setWalFilter(JNIEnv*, jclass, jlong jhandle,
20552055
opt->wal_filter = wal_filter;
20562056
}
20572057

2058-
/*
2059-
* Class: org_rocksdb_Options
2060-
* Method: setFailIfOptionsFileError
2061-
* Signature: (JZ)V
2062-
*/
2063-
void Java_org_rocksdb_Options_setFailIfOptionsFileError(
2064-
JNIEnv*, jclass, jlong jhandle, jboolean jfail_if_options_file_error) {
2065-
auto* opt = reinterpret_cast<ROCKSDB_NAMESPACE::Options*>(jhandle);
2066-
opt->fail_if_options_file_error =
2067-
static_cast<bool>(jfail_if_options_file_error);
2068-
}
2069-
2070-
/*
2071-
* Class: org_rocksdb_Options
2072-
* Method: failIfOptionsFileError
2073-
* Signature: (J)Z
2074-
*/
2075-
jboolean Java_org_rocksdb_Options_failIfOptionsFileError(JNIEnv*, jclass,
2076-
jlong jhandle) {
2077-
auto* opt = reinterpret_cast<ROCKSDB_NAMESPACE::Options*>(jhandle);
2078-
return static_cast<jboolean>(opt->fail_if_options_file_error);
2079-
}
2080-
20812058
/*
20822059
* Class: org_rocksdb_Options
20832060
* Method: setDumpMallocStats
@@ -7479,29 +7456,6 @@ void Java_org_rocksdb_DBOptions_setWalFilter(JNIEnv*, jclass, jlong jhandle,
74797456
opt->wal_filter = wal_filter;
74807457
}
74817458

7482-
/*
7483-
* Class: org_rocksdb_DBOptions
7484-
* Method: setFailIfOptionsFileError
7485-
* Signature: (JZ)V
7486-
*/
7487-
void Java_org_rocksdb_DBOptions_setFailIfOptionsFileError(
7488-
JNIEnv*, jclass, jlong jhandle, jboolean jfail_if_options_file_error) {
7489-
auto* opt = reinterpret_cast<ROCKSDB_NAMESPACE::DBOptions*>(jhandle);
7490-
opt->fail_if_options_file_error =
7491-
static_cast<bool>(jfail_if_options_file_error);
7492-
}
7493-
7494-
/*
7495-
* Class: org_rocksdb_DBOptions
7496-
* Method: failIfOptionsFileError
7497-
* Signature: (J)Z
7498-
*/
7499-
jboolean Java_org_rocksdb_DBOptions_failIfOptionsFileError(JNIEnv*, jclass,
7500-
jlong jhandle) {
7501-
auto* opt = reinterpret_cast<ROCKSDB_NAMESPACE::DBOptions*>(jhandle);
7502-
return static_cast<jboolean>(opt->fail_if_options_file_error);
7503-
}
7504-
75057459
/*
75067460
* Class: org_rocksdb_DBOptions
75077461
* Method: setDumpMallocStats

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

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -656,15 +656,6 @@ public String name() {
656656
}
657657
}
658658

659-
@Test
660-
public void failIfOptionsFileError() {
661-
try (final DBOptions opt = new DBOptions()) {
662-
final boolean boolValue = rand.nextBoolean();
663-
opt.setFailIfOptionsFileError(boolValue);
664-
assertThat(opt.failIfOptionsFileError()).isEqualTo(boolValue);
665-
}
666-
}
667-
668659
@Test
669660
public void dumpMallocStats() {
670661
try (final DBOptions opt = new DBOptions()) {

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

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -902,15 +902,6 @@ public String name() {
902902
}
903903
}
904904

905-
@Test
906-
public void failIfOptionsFileError() {
907-
try (final Options opt = new Options()) {
908-
final boolean boolValue = rand.nextBoolean();
909-
opt.setFailIfOptionsFileError(boolValue);
910-
assertThat(opt.failIfOptionsFileError()).isEqualTo(boolValue);
911-
}
912-
}
913-
914905
@Test
915906
public void dumpMallocStats() {
916907
try (final Options opt = new Options()) {

‎options/db_options.cc

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -319,8 +319,7 @@ static std::unordered_map<std::string, OptionTypeInfo>
319319
OptionType::kBoolean, OptionVerificationType::kNormal,
320320
OptionTypeFlags::kNone}},
321321
{"fail_if_options_file_error",
322-
{offsetof(struct ImmutableDBOptions, fail_if_options_file_error),
323-
OptionType::kBoolean, OptionVerificationType::kNormal,
322+
{0, OptionType::kBoolean, OptionVerificationType::kDeprecated,
324323
OptionTypeFlags::kNone}},
325324
{"enable_pipelined_write",
326325
{offsetof(struct ImmutableDBOptions, enable_pipelined_write),
@@ -772,7 +771,6 @@ ImmutableDBOptions::ImmutableDBOptions(const DBOptions& options)
772771
allow_2pc(options.allow_2pc),
773772
row_cache(options.row_cache),
774773
wal_filter(options.wal_filter),
775-
fail_if_options_file_error(options.fail_if_options_file_error),
776774
dump_malloc_stats(options.dump_malloc_stats),
777775
avoid_flush_during_recovery(options.avoid_flush_during_recovery),
778776
allow_ingest_behind(options.allow_ingest_behind),

‎options/db_options.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ struct ImmutableDBOptions {
7777
bool allow_2pc;
7878
std::shared_ptr<Cache> row_cache;
7979
WalFilter* wal_filter;
80-
bool fail_if_options_file_error;
8180
bool dump_malloc_stats;
8281
bool avoid_flush_during_recovery;
8382
bool allow_ingest_behind;

‎options/options_helper.cc

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,6 @@ void BuildDBOptions(const ImmutableDBOptions& immutable_db_options,
153153
options.allow_2pc = immutable_db_options.allow_2pc;
154154
options.row_cache = immutable_db_options.row_cache;
155155
options.wal_filter = immutable_db_options.wal_filter;
156-
options.fail_if_options_file_error =
157-
immutable_db_options.fail_if_options_file_error;
158156
options.dump_malloc_stats = immutable_db_options.dump_malloc_stats;
159157
options.avoid_flush_during_recovery =
160158
immutable_db_options.avoid_flush_during_recovery;

‎options/options_settable_test.cc

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,6 @@ TEST_F(OptionsSettableTest, DBOptionsAllFieldsSettable) {
433433
"use_direct_io_for_flush_and_compaction=false;"
434434
"max_log_file_size=4607;"
435435
"advise_random_on_open=true;"
436-
"fail_if_options_file_error=false;"
437436
"enable_pipelined_write=false;"
438437
"unordered_write=false;"
439438
"allow_concurrent_memtable_write=true;"

‎tools/db_crashtest.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,6 @@
9191
# (see below `finalize_and_sanitize`).
9292
"inplace_update_support": random.choice([0] * 9 + [1]),
9393
"expected_values_dir": lambda: setup_expected_values_dir(),
94-
"fail_if_options_file_error": lambda: random.randint(0, 1),
9594
"flush_one_in": lambda: random.choice([1000, 1000000]),
9695
"manual_wal_flush_one_in": lambda: random.choice([0, 1000]),
9796
"file_checksum_impl": lambda: random.choice(["none", "crc32c", "xxh64", "big"]),

‎unreleased_history/public_api_changes/remove_fail_if_options_file_error.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
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

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -847,54 +847,6 @@ TEST_P(CheckpointTestWithWalParams, CheckpointWithUnsyncedDataDropped) {
847847
db_ = nullptr;
848848
}
849849

850-
TEST_F(CheckpointTest, CheckpointOptionsFileFailedToPersist) {
851-
// Regression test for a bug where checkpoint failed on a DB where persisting
852-
// OPTIONS file failed and the DB was opened with
853-
// `fail_if_options_file_error == false`.
854-
Options options = CurrentOptions();
855-
options.fail_if_options_file_error = false;
856-
auto fault_fs = std::make_shared<FaultInjectionTestFS>(FileSystem::Default());
857-
858-
// Setup `FaultInjectionTestFS` and `SyncPoint` callbacks to fail one
859-
// operation when inside the OPTIONS file persisting code.
860-
std::unique_ptr<Env> fault_fs_env(NewCompositeEnv(fault_fs));
861-
fault_fs->SetThreadLocalErrorContext(
862-
FaultInjectionIOType::kWrite, 7 /* seed*/, 1 /* one_in */,
863-
false /* retryable */, false /* has_data_loss*/);
864-
SyncPoint::GetInstance()->SetCallBack(
865-
"PersistRocksDBOptions:start", [fault_fs](void* /* arg */) {
866-
fault_fs->EnableThreadLocalErrorInjection(
867-
FaultInjectionIOType::kMetadataWrite);
868-
});
869-
SyncPoint::GetInstance()->SetCallBack(
870-
"FaultInjectionTestFS::InjectMetadataWriteError:Injected",
871-
[fault_fs](void* /* arg */) {
872-
fault_fs->DisableThreadLocalErrorInjection(
873-
FaultInjectionIOType::kMetadataWrite);
874-
});
875-
options.env = fault_fs_env.get();
876-
SyncPoint::GetInstance()->EnableProcessing();
877-
878-
Reopen(options);
879-
ASSERT_OK(Put("key1", "val1"));
880-
Checkpoint* checkpoint;
881-
ASSERT_OK(Checkpoint::Create(db_, &checkpoint));
882-
ASSERT_OK(checkpoint->CreateCheckpoint(snapshot_name_));
883-
delete checkpoint;
884-
885-
// Make sure it's usable.
886-
options.env = env_;
887-
DB* snapshot_db;
888-
ASSERT_OK(DB::Open(options, snapshot_name_, &snapshot_db));
889-
ReadOptions read_opts;
890-
std::string get_result;
891-
ASSERT_OK(snapshot_db->Get(read_opts, "key1", &get_result));
892-
ASSERT_EQ("val1", get_result);
893-
delete snapshot_db;
894-
delete db_;
895-
db_ = nullptr;
896-
}
897-
898850
TEST_F(CheckpointTest, CheckpointReadOnlyDB) {
899851
ASSERT_OK(Put("foo", "foo_value"));
900852
ASSERT_OK(Flush());

‎utilities/ttl/ttl_test.cc

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -617,7 +617,6 @@ TEST_F(TtlTest, UnregisteredMergeOperator) {
617617
public:
618618
const char* Name() const override { return "UnregisteredMergeOperator"; }
619619
};
620-
options_.fail_if_options_file_error = true;
621620
options_.merge_operator = std::make_shared<UnregisteredMergeOperator>();
622621
OpenTtl();
623622
CloseTtl();

0 commit comments

Comments
 (0)