From 4624edc440a1623c2e548db7fd3db33a6380f4cc Mon Sep 17 00:00:00 2001 From: Siying Dong Date: Thu, 22 Feb 2018 13:23:53 -0800 Subject: [PATCH] RocksDBOptionsParser::Parse()'s `ignore_unknown_options` argument only ingores options from higher version. Summary: RocksDB should always be able to parse an option file generated using the same or lower version. Unknown option should only happen if it is from a higher version. Change the behavior of RocksDBOptionsParser::Parse()'s behavior with ignore_unknown_options=true so that unknown option from a lower or the same version will never be skipped. Closes https://github.com/facebook/rocksdb/pull/3527 Differential Revision: D7048851 Pulled By: siying fbshipit-source-id: e261caea12f6515611a4a29f39acf2b619df2361 --- HISTORY.md | 1 + options/options_parser.cc | 10 ++++ options/options_test.cc | 100 +++++++++++++++++++++++++++----------- 3 files changed, 82 insertions(+), 29 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 61e5566c3..1627bcb4e 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -5,6 +5,7 @@ * Add `include_end` option to make the range end exclusive when `include_end == false` in `DeleteFilesInRange()`. * Add `CompactRangeOptions::allow_write_stall`, which makes `CompactRange` start working immediately, even if it causes user writes to stall. The default value is false, meaning we add delay to `CompactRange` calls until stalling can be avoided when possible. Note this delay is not present in previous RocksDB versions. * Creating checkpoint with empty directory now returns `Status::InvalidArgument`; previously, it returned `Status::IOError`. +* RocksDBOptionsParser::Parse()'s `ignore_unknown_options` argument will only be effective if the option file shows it is generated using a higher version of RocksDB than the current version. ### New Features * Improve the performance of iterators doing long range scans by using readahead. diff --git a/options/options_parser.cc b/options/options_parser.cc index 040e51c92..0095c3ab7 100644 --- a/options/options_parser.cc +++ b/options/options_parser.cc @@ -268,6 +268,16 @@ Status RocksDBOptionsParser::Parse(const std::string& file_name, Env* env, if (!s.ok()) { return s; } + + // If the option file is not generated by a higher minor version, + // there shouldn't be any unknown option. + if (ignore_unknown_options && section == kOptionSectionVersion) { + if (db_version[0] < ROCKSDB_MAJOR || (db_version[0] == ROCKSDB_MAJOR && + db_version[1] <= ROCKSDB_MINOR)) { + ignore_unknown_options = false; + } + } + s = ParseSection(§ion, &title, &argument, line, line_num); if (!s.ok()) { return s; diff --git a/options/options_test.cc b/options/options_test.cc index edbfd3cfb..0688d3554 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -1216,37 +1216,79 @@ TEST_F(OptionsParserTest, DuplicateCFOptions) { } TEST_F(OptionsParserTest, IgnoreUnknownOptions) { - DBOptions db_opt; - db_opt.max_open_files = 12345; - db_opt.max_background_flushes = 301; - db_opt.max_total_wal_size = 1024; - ColumnFamilyOptions cf_opt; + for (int case_id = 0; case_id < 5; case_id++) { + DBOptions db_opt; + db_opt.max_open_files = 12345; + db_opt.max_background_flushes = 301; + db_opt.max_total_wal_size = 1024; + ColumnFamilyOptions cf_opt; - std::string options_file_content = - "# This is a testing option string.\n" - "# Currently we only support \"#\" styled comment.\n" - "\n" - "[Version]\n" - " rocksdb_version=3.14.0\n" - " options_file_version=1\n" - "[DBOptions]\n" - " max_open_files=12345\n" - " max_background_flushes=301\n" - " max_total_wal_size=1024 # keep_log_file_num=1000\n" - " unknown_db_option1=321\n" - " unknown_db_option2=false\n" - "[CFOptions \"default\"]\n" - " unknown_cf_option1=hello\n" - "[CFOptions \"something_else\"]\n" - " unknown_cf_option2=world\n" - " # if a section is blank, we will use the default\n"; + std::string version_string; + bool should_ignore = true; + if (case_id == 0) { + // same version + should_ignore = false; + version_string = + ToString(ROCKSDB_MAJOR) + "." + ToString(ROCKSDB_MINOR) + ".0"; + } else if (case_id == 1) { + // higher minor version + should_ignore = true; + version_string = + ToString(ROCKSDB_MAJOR) + "." + ToString(ROCKSDB_MINOR + 1) + ".0"; + } else if (case_id == 2) { + // higher major version. + should_ignore = true; + version_string = ToString(ROCKSDB_MAJOR + 1) + ".0.0"; + } else if (case_id == 3) { + // lower minor version +#if ROCKSDB_MINOR == 0 + continue; +#else + version_string = + ToString(ROCKSDB_MAJOR) + "." + ToString(ROCKSDB_MINOR - 1) + ".0"; + should_ignore = false; +#endif + } else { + // lower major version + should_ignore = false; + version_string = + ToString(ROCKSDB_MAJOR - 1) + "." + ToString(ROCKSDB_MINOR) + ".0"; + } - const std::string kTestFileName = "test-rocksdb-options.ini"; - env_->WriteToNewFile(kTestFileName, options_file_content); - RocksDBOptionsParser parser; - ASSERT_NOK(parser.Parse(kTestFileName, env_.get())); - ASSERT_OK(parser.Parse(kTestFileName, env_.get(), - true /* ignore_unknown_options */)); + std::string options_file_content = + "# This is a testing option string.\n" + "# Currently we only support \"#\" styled comment.\n" + "\n" + "[Version]\n" + " rocksdb_version=" + + version_string + + "\n" + " options_file_version=1\n" + "[DBOptions]\n" + " max_open_files=12345\n" + " max_background_flushes=301\n" + " max_total_wal_size=1024 # keep_log_file_num=1000\n" + " unknown_db_option1=321\n" + " unknown_db_option2=false\n" + "[CFOptions \"default\"]\n" + " unknown_cf_option1=hello\n" + "[CFOptions \"something_else\"]\n" + " unknown_cf_option2=world\n" + " # if a section is blank, we will use the default\n"; + + const std::string kTestFileName = "test-rocksdb-options.ini"; + env_->DeleteFile(kTestFileName); + env_->WriteToNewFile(kTestFileName, options_file_content); + RocksDBOptionsParser parser; + ASSERT_NOK(parser.Parse(kTestFileName, env_.get())); + if (should_ignore) { + ASSERT_OK(parser.Parse(kTestFileName, env_.get(), + true /* ignore_unknown_options */)); + } else { + ASSERT_NOK(parser.Parse(kTestFileName, env_.get(), + true /* ignore_unknown_options */)); + } + } } TEST_F(OptionsParserTest, ParseVersion) {