Skip to content

Commit

Permalink
Revert LoadLatestOptions handling of ignore_unknown_options if versio…
Browse files Browse the repository at this point in the history 
…ns differ (facebook#7612)

Summary: Pull Request resolved: facebook#7612

Reviewed By: zhichao-cao

Differential Revision: D24627054

Pulled By: riversand963

fbshipit-source-id: 451b4da742e3e84c7442bc7cc4959d39089b89d0
  • Loading branch information
@mrambacher @facebook-github-bot
mrambacher authored and facebook-github-bot committed Oct 29, 2020
1 parent 394210f commit 7eb2824
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 19 deletions.
12 changes: 6 additions & 6 deletions options/options_parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,13 +289,13 @@ Status RocksDBOptionsParser::Parse(const ConfigOptions& config_options_in,
return s;
}

// If the option file is newer than the current version
// there may be unknown options.
if (!config_options.ignore_unknown_options &&
// If the option file is not generated by a higher minor version,
// there shouldn't be any unknown option.
if (config_options.ignore_unknown_options &&
section == kOptionSectionVersion) {
if (db_version[0] > ROCKSDB_MAJOR ||
(db_version[0] == ROCKSDB_MAJOR && db_version[1] > ROCKSDB_MINOR)) {
config_options.ignore_unknown_options = true;
if (db_version[0] < ROCKSDB_MAJOR || (db_version[0] == ROCKSDB_MAJOR &&
db_version[1] <= ROCKSDB_MINOR)) {
config_options.ignore_unknown_options = false;
}
}

Expand Down
8 changes: 4 additions & 4 deletions options/options_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2665,15 +2665,15 @@ TEST_F(OptionsParserTest, IgnoreUnknownOptions) {
}
ASSERT_OK(env_->WriteToNewFile(kTestFileName, options_file_content));
RocksDBOptionsParser parser;
ASSERT_OK(parser.Parse(kTestFileName, fs_.get(), true,
4096 /* readahead_size */));
ASSERT_NOK(parser.Parse(kTestFileName, fs_.get(), false,
4096 /* readahead_size */));
if (should_ignore) {
ASSERT_OK(parser.Parse(kTestFileName, fs_.get(),
false /* ignore_unknown_options */,
true /* ignore_unknown_options */,
4096 /* readahead_size */));
} else {
ASSERT_NOK(parser.Parse(kTestFileName, fs_.get(),
false /* ignore_unknown_options */,
true /* ignore_unknown_options */,
4096 /* readahead_size */));
}
}
Expand Down
34 changes: 25 additions & 9 deletions utilities/options/options_util_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -552,57 +552,73 @@ TEST_F(OptionsUtilTest, BadLatestOptions) {
s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs);
ASSERT_NOK(s);
ASSERT_TRUE(s.IsInvalidArgument());
ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs));

// Even though ignore_unknown_options=true, we still return an error...
s = LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs);
ASSERT_NOK(s);
ASSERT_TRUE(s.IsInvalidArgument());
// Write an options file for a previous minor release with an unknown CF
// Option
WriteOptionsFile(options.env, dbname_, "OPTIONS-0002", ROCKSDB_MAJOR,
ROCKSDB_MINOR - 1, "", "unknown_cf_opt=true");
s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs);
ASSERT_NOK(s);
ASSERT_TRUE(s.IsInvalidArgument());
ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs));
// Even though ignore_unknown_options=true, we still return an error...
s = LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs);
ASSERT_NOK(s);
ASSERT_TRUE(s.IsInvalidArgument());

// Write an options file for the current release with an unknown DB Option
WriteOptionsFile(options.env, dbname_, "OPTIONS-0003", ROCKSDB_MAJOR,
ROCKSDB_MINOR, "unknown_db_opt=true", "");
s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs);
ASSERT_NOK(s);
ASSERT_TRUE(s.IsInvalidArgument());
ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs));
// Even though ignore_unknown_options=true, we still return an error...
s = LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs);
ASSERT_NOK(s);
ASSERT_TRUE(s.IsInvalidArgument());

// Write an options file for the current release with an unknown CF Option
WriteOptionsFile(options.env, dbname_, "OPTIONS-0004", ROCKSDB_MAJOR,
ROCKSDB_MINOR, "", "unknown_cf_opt=true");
s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs);
ASSERT_NOK(s);
ASSERT_TRUE(s.IsInvalidArgument());
ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs));
// Even though ignore_unknown_options=true, we still return an error...
s = LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs);
ASSERT_NOK(s);
ASSERT_TRUE(s.IsInvalidArgument());

// Write an options file for the current release with an invalid DB Option
WriteOptionsFile(options.env, dbname_, "OPTIONS-0005", ROCKSDB_MAJOR,
ROCKSDB_MINOR, "create_if_missing=hello", "");
s = LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs);
ASSERT_NOK(s);
ASSERT_TRUE(s.IsInvalidArgument());
ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs));
// Even though ignore_unknown_options=true, we still return an error...
s = LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs);
ASSERT_NOK(s);
ASSERT_TRUE(s.IsInvalidArgument());

// Write an options file for the next release with an invalid DB Option
WriteOptionsFile(options.env, dbname_, "OPTIONS-0006", ROCKSDB_MAJOR,
ROCKSDB_MINOR + 1, "create_if_missing=hello", "");
ASSERT_OK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs));
ASSERT_NOK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs));
ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs));

// Write an options file for the next release with an unknown DB Option
WriteOptionsFile(options.env, dbname_, "OPTIONS-0007", ROCKSDB_MAJOR,
ROCKSDB_MINOR + 1, "unknown_db_opt=true", "");
ASSERT_OK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs));
ASSERT_NOK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs));
// Ignore the errors for future releases when ignore_unknown_options=true
ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs));

// Write an options file for the next major release with an unknown CF Option
WriteOptionsFile(options.env, dbname_, "OPTIONS-0008", ROCKSDB_MAJOR + 1,
ROCKSDB_MINOR, "", "unknown_cf_opt=true");
ASSERT_OK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs));
ASSERT_NOK(LoadLatestOptions(config_opts, dbname_, &db_opts, &cf_descs));
// Ignore the errors for future releases when ignore_unknown_options=true
ASSERT_OK(LoadLatestOptions(ignore_opts, dbname_, &db_opts, &cf_descs));
}
} // namespace ROCKSDB_NAMESPACE
Expand Down

0 comments on commit 7eb2824

Please sign in to comment.