From 79ca039eb44546ddb71c1f6ba6626dc614a662a7 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Fri, 19 Feb 2016 14:42:24 -0800 Subject: [PATCH] Relax the check condition of prefix_extractor in CheckOptionsCompatibility Summary: Relax the check condition of prefix_extractor in CheckOptionsCompatibility by allowing changing value from non-nullptr to nullptr or nullptr to non-nullptr. Test Plan: options_test options_util_test Reviewers: sdong, anthony, IslamAbdelRahman, kradhakrishnan, gunnarku Reviewed By: gunnarku Subscribers: dhruba, leveldb Differential Revision: https://reviews.facebook.net/D54477 --- util/options_helper.cc | 12 +++++++++--- util/options_helper.h | 18 +++++++++-------- util/options_parser.cc | 10 +++++++++- util/options_test.cc | 18 +++++++++++++---- utilities/options/options_util_test.cc | 27 ++++++++++++++++++++++++-- 5 files changed, 67 insertions(+), 18 deletions(-) diff --git a/util/options_helper.cc b/util/options_helper.cc index f2929a740..44b57f48b 100644 --- a/util/options_helper.cc +++ b/util/options_helper.cc @@ -805,6 +805,7 @@ Status ParseColumnFamilyOption(const std::string& name, } switch (opt_info.verification) { case OptionVerificationType::kByName: + case OptionVerificationType::kByNameAllowNull: return Status::NotSupported( "Deserializing the specified CF option " + name + " is not supported"); @@ -985,6 +986,7 @@ Status ParseDBOption(const std::string& name, } switch (opt_info.verification) { case OptionVerificationType::kByName: + case OptionVerificationType::kByNameAllowNull: return Status::NotSupported( "Deserializing the specified DB option " + name + " is not supported"); @@ -1082,6 +1084,8 @@ Status GetBlockBasedTableOptionsFromMap( // the old API, where everything is // parsable. (iter->second.verification != OptionVerificationType::kByName && + iter->second.verification != + OptionVerificationType::kByNameAllowNull && iter->second.verification != OptionVerificationType::kDeprecated)) { return Status::InvalidArgument("Can't parse BlockBasedTableOptions:", o.first + " " + error_message); @@ -1116,10 +1120,12 @@ Status GetPlainTableOptionsFromMap( if (error_message != "") { const auto iter = plain_table_type_info.find(o.first); if (iter == plain_table_type_info.end() || - !input_strings_escaped ||// !input_strings_escaped indicates - // the old API, where everything is - // parsable. + !input_strings_escaped || // !input_strings_escaped indicates + // the old API, where everything is + // parsable. (iter->second.verification != OptionVerificationType::kByName && + iter->second.verification != + OptionVerificationType::kByNameAllowNull && iter->second.verification != OptionVerificationType::kDeprecated)) { return Status::InvalidArgument("Can't parse PlainTableOptions:", o.first + " " + error_message); diff --git a/util/options_helper.h b/util/options_helper.h index 1c8b585d6..fc7e2c2e2 100644 --- a/util/options_helper.h +++ b/util/options_helper.h @@ -98,13 +98,15 @@ enum class OptionType { enum class OptionVerificationType { kNormal, - kByName, // The option is pointer typed so we can only verify - // based on it's name. - kDeprecated // The option is no longer used in rocksdb. The RocksDB - // OptionsParser will still accept this option if it - // happen to exists in some Options file. However, the - // parser will not include it in serialization and - // verification processes. + kByName, // The option is pointer typed so we can only verify + // based on it's name. + kByNameAllowNull, // Same as kByName, but it also allows the case + // where one of them is a nullptr. + kDeprecated // The option is no longer used in rocksdb. The RocksDB + // OptionsParser will still accept this option if it + // happen to exists in some Options file. However, the + // parser will not include it in serialization and + // verification processes. }; // A struct for storing constant option information such as option name, @@ -433,7 +435,7 @@ static std::unordered_map cf_options_type_info = { OptionVerificationType::kByName}}, {"prefix_extractor", {offsetof(struct ColumnFamilyOptions, prefix_extractor), - OptionType::kSliceTransform, OptionVerificationType::kByName}}, + OptionType::kSliceTransform, OptionVerificationType::kByNameAllowNull}}, {"memtable_factory", {offsetof(struct ColumnFamilyOptions, memtable_factory), OptionType::kMemTableRepFactory, OptionVerificationType::kByName}}, diff --git a/util/options_parser.cc b/util/options_parser.cc index e5689fdc4..e01529bff 100644 --- a/util/options_parser.cc +++ b/util/options_parser.cc @@ -510,6 +510,7 @@ bool AreEqualOptions( const std::unordered_map* opt_map) { const char* offset1 = opt1 + type_info.offset; const char* offset2 = opt2 + type_info.offset; + static const std::string kNullptrString = "nullptr"; switch (type_info.type) { case OptionType::kBoolean: return (*reinterpret_cast(offset1) == @@ -557,7 +558,8 @@ bool AreEqualOptions( offset1) == *reinterpret_cast(offset2)); default: - if (type_info.verification == OptionVerificationType::kByName) { + if (type_info.verification == OptionVerificationType::kByName || + type_info.verification == OptionVerificationType::kByNameAllowNull) { std::string value1; bool result = SerializeSingleOptionHelper(offset1, type_info.type, &value1); @@ -571,6 +573,12 @@ bool AreEqualOptions( if (iter == opt_map->end()) { return true; } else { + if (type_info.verification == + OptionVerificationType::kByNameAllowNull) { + if (iter->second == kNullptrString || value1 == kNullptrString) { + return true; + } + } return (value1 == iter->second); } } diff --git a/util/options_test.cc b/util/options_test.cc index 0bde6d618..02f128d69 100644 --- a/util/options_test.cc +++ b/util/options_test.cc @@ -1308,10 +1308,10 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) { // prefix_extractor { - // change the prefix extractor and expect only pass when - // sanity-level == kSanityLevelNone + // Okay to change prefix_extractor form nullptr to non-nullptr + ASSERT_EQ(opts.prefix_extractor.get(), nullptr); opts.prefix_extractor.reset(NewCappedPrefixTransform(10)); - ASSERT_NOK(SanityCheckCFOptions(opts, kSanityLevelLooselyCompatible)); + ASSERT_OK(SanityCheckCFOptions(opts, kSanityLevelLooselyCompatible)); ASSERT_OK(SanityCheckCFOptions(opts, kSanityLevelNone)); // persist the change @@ -1338,11 +1338,21 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) { // expect pass only in kSanityLevelNone ASSERT_NOK(SanityCheckCFOptions(opts, kSanityLevelLooselyCompatible)); ASSERT_OK(SanityCheckCFOptions(opts, kSanityLevelNone)); + + // Change prefix extractor from non-nullptr to nullptr + opts.prefix_extractor.reset(); + // expect pass as it's safe to change prefix_extractor + // from non-null to null + ASSERT_OK(SanityCheckCFOptions(opts, kSanityLevelLooselyCompatible)); + ASSERT_OK(SanityCheckCFOptions(opts, kSanityLevelNone)); } + // persist the change + ASSERT_OK(PersistCFOptions(opts)); + ASSERT_OK(SanityCheckCFOptions(opts, kSanityLevelExactMatch)); // table_factory { - for (int tb = 2; tb >= 0; --tb) { + for (int tb = 0; tb <= 2; ++tb) { // change the table factory opts.table_factory.reset(test::RandomTableFactory(&rnd, tb)); ASSERT_NOK(SanityCheckCFOptions(opts, kSanityLevelLooselyCompatible)); diff --git a/utilities/options/options_util_test.cc b/utilities/options/options_util_test.cc index e93d8a837..94ddbc408 100644 --- a/utilities/options/options_util_test.cc +++ b/utilities/options/options_util_test.cc @@ -173,8 +173,9 @@ TEST_F(OptionsUtilTest, SanityCheck) { (i == 0) ? kDefaultColumnFamilyName : test::RandomName(&rnd_, 10); cf_descs.back().options.table_factory.reset(NewBlockBasedTableFactory()); + // Assign non-null values to prefix_extractors except the first cf. cf_descs.back().options.prefix_extractor.reset( - test::RandomSliceTransform(&rnd_)); + i != 0 ? test::RandomSliceTransform(&rnd_) : nullptr); cf_descs.back().options.merge_operator.reset( test::RandomMergeOperator(&rnd_)); } @@ -223,9 +224,10 @@ TEST_F(OptionsUtilTest, SanityCheck) { std::shared_ptr prefix_extractor = cf_descs[1].options.prefix_extractor; + // It's okay to set prefix_extractor to nullptr. ASSERT_NE(prefix_extractor, nullptr); cf_descs[1].options.prefix_extractor.reset(); - ASSERT_NOK( + ASSERT_OK( CheckOptionsCompatibility(dbname_, Env::Default(), db_opt, cf_descs)); cf_descs[1].options.prefix_extractor.reset(new DummySliceTransform()); @@ -237,6 +239,27 @@ TEST_F(OptionsUtilTest, SanityCheck) { CheckOptionsCompatibility(dbname_, Env::Default(), db_opt, cf_descs)); } + // prefix extractor nullptr case + { + std::shared_ptr prefix_extractor = + cf_descs[0].options.prefix_extractor; + + // It's okay to set prefix_extractor to nullptr. + ASSERT_EQ(prefix_extractor, nullptr); + cf_descs[0].options.prefix_extractor.reset(); + ASSERT_OK( + CheckOptionsCompatibility(dbname_, Env::Default(), db_opt, cf_descs)); + + // It's okay to change prefix_extractor from nullptr to non-nullptr + cf_descs[0].options.prefix_extractor.reset(new DummySliceTransform()); + ASSERT_OK( + CheckOptionsCompatibility(dbname_, Env::Default(), db_opt, cf_descs)); + + cf_descs[0].options.prefix_extractor = prefix_extractor; + ASSERT_OK( + CheckOptionsCompatibility(dbname_, Env::Default(), db_opt, cf_descs)); + } + // comparator { test::SimpleSuffixReverseComparator comparator;