From 7eb2824e3f33534120cbbcc694749f1edc6e748e Mon Sep 17 00:00:00 2001 From: mrambacher Date: Thu, 29 Oct 2020 13:45:17 -0700 Subject: [PATCH] Revert LoadLatestOptions handling of ignore_unknown_options if versions differ (#7612) Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/7612 Reviewed By: zhichao-cao Differential Revision: D24627054 Pulled By: riversand963 fbshipit-source-id: 451b4da742e3e84c7442bc7cc4959d39089b89d0 --- options/options_parser.cc | 12 ++++----- options/options_test.cc | 8 +++--- utilities/options/options_util_test.cc | 34 +++++++++++++++++++------- 3 files changed, 35 insertions(+), 19 deletions(-) diff --git a/options/options_parser.cc b/options/options_parser.cc index 6642d4115..4ce578b41 100644 --- a/options/options_parser.cc +++ b/options/options_parser.cc @@ -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; } } diff --git a/options/options_test.cc b/options/options_test.cc index f568208aa..ffa91f2cd 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -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 */)); } } diff --git a/utilities/options/options_util_test.cc b/utilities/options/options_util_test.cc index 06ae9e62c..ca96ad645 100644 --- a/utilities/options/options_util_test.cc +++ b/utilities/options/options_util_test.cc @@ -552,8 +552,10 @@ 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, @@ -561,7 +563,10 @@ 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 the current release with an unknown DB Option WriteOptionsFile(options.env, dbname_, "OPTIONS-0003", ROCKSDB_MAJOR, @@ -569,7 +574,10 @@ 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 the current release with an unknown CF Option WriteOptionsFile(options.env, dbname_, "OPTIONS-0004", ROCKSDB_MAJOR, @@ -577,7 +585,10 @@ 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 the current release with an invalid DB Option WriteOptionsFile(options.env, dbname_, "OPTIONS-0005", ROCKSDB_MAJOR, @@ -585,24 +596,29 @@ 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 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