From 8fb3fe8d39a793d8881028efe19375621ac64faf Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 19 Oct 2021 10:42:04 -0700 Subject: [PATCH] Allow unregistered options to be ignored in DBOptions from files (#9045) Summary: Adds changes to DBOptions (comparable to ColumnFamilyOptions) to allow some option values to be ignored on rehydration from the Options file. This is necessary for some customizable classes that were not registered with the ObjectRegistry but are saved/restored from the Options file. All tests pass. Will run check_format_compatible.sh shortly. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9045 Reviewed By: zhichao-cao Differential Revision: D31761664 Pulled By: mrambacher fbshipit-source-id: 300c2251639cce2b223481c3bb2a63877b1f3766 --- options/customizable_test.cc | 23 +++--- options/db_options.cc | 61 ++++++++++++-- options/options_helper.h | 4 +- options/options_parser.cc | 12 ++- options/options_test.cc | 154 ++++++++++++++++++++++++++--------- util/slice.cc | 74 ++++++++--------- 6 files changed, 231 insertions(+), 97 deletions(-) diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 806a368a6..4ca04beff 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -1463,7 +1463,6 @@ class LoadCustomizableTest : public testing::Test { }; TEST_F(LoadCustomizableTest, LoadTableFactoryTest) { - ColumnFamilyOptions cf_opts; std::shared_ptr factory; ASSERT_NOK(TableFactory::CreateFromString( config_options_, mock::MockTableFactory::kClassName(), &factory)); @@ -1474,10 +1473,10 @@ TEST_F(LoadCustomizableTest, LoadTableFactoryTest) { #ifndef ROCKSDB_LITE std::string opts_str = "table_factory="; ASSERT_OK(GetColumnFamilyOptionsFromString( - config_options_, ColumnFamilyOptions(), - opts_str + TableFactory::kBlockBasedTableName(), &cf_opts)); - ASSERT_NE(cf_opts.table_factory.get(), nullptr); - ASSERT_STREQ(cf_opts.table_factory->Name(), + config_options_, cf_opts_, + opts_str + TableFactory::kBlockBasedTableName(), &cf_opts_)); + ASSERT_NE(cf_opts_.table_factory.get(), nullptr); + ASSERT_STREQ(cf_opts_.table_factory->Name(), TableFactory::kBlockBasedTableName()); #endif // ROCKSDB_LITE if (RegisterTests("Test")) { @@ -1487,10 +1486,10 @@ TEST_F(LoadCustomizableTest, LoadTableFactoryTest) { ASSERT_STREQ(factory->Name(), mock::MockTableFactory::kClassName()); #ifndef ROCKSDB_LITE ASSERT_OK(GetColumnFamilyOptionsFromString( - config_options_, ColumnFamilyOptions(), - opts_str + mock::MockTableFactory::kClassName(), &cf_opts)); - ASSERT_NE(cf_opts.table_factory.get(), nullptr); - ASSERT_STREQ(cf_opts.table_factory->Name(), + config_options_, cf_opts_, + opts_str + mock::MockTableFactory::kClassName(), &cf_opts_)); + ASSERT_NE(cf_opts_.table_factory.get(), nullptr); + ASSERT_STREQ(cf_opts_.table_factory->Name(), mock::MockTableFactory::kClassName()); #endif // ROCKSDB_LITE } @@ -1763,12 +1762,12 @@ TEST_F(LoadCustomizableTest, LoadEncryptionProviderTest) { EncryptionProvider::CreateFromString(config_options_, "CTR", &result)); ASSERT_NE(result, nullptr); ASSERT_STREQ(result->Name(), "CTR"); - ASSERT_NOK(result->ValidateOptions(DBOptions(), ColumnFamilyOptions())); + ASSERT_NOK(result->ValidateOptions(db_opts_, cf_opts_)); ASSERT_OK(EncryptionProvider::CreateFromString(config_options_, "CTR://test", &result)); ASSERT_NE(result, nullptr); ASSERT_STREQ(result->Name(), "CTR"); - ASSERT_OK(result->ValidateOptions(DBOptions(), ColumnFamilyOptions())); + ASSERT_OK(result->ValidateOptions(db_opts_, cf_opts_)); if (RegisterTests("Test")) { ASSERT_OK( @@ -1779,7 +1778,7 @@ TEST_F(LoadCustomizableTest, LoadEncryptionProviderTest) { "Mock://test", &result)); ASSERT_NE(result, nullptr); ASSERT_STREQ(result->Name(), "Mock"); - ASSERT_OK(result->ValidateOptions(DBOptions(), ColumnFamilyOptions())); + ASSERT_OK(result->ValidateOptions(db_opts_, cf_opts_)); } } diff --git a/options/db_options.cc b/options/db_options.cc index 7a91c9e6d..d99df242c 100644 --- a/options/db_options.cc +++ b/options/db_options.cc @@ -452,7 +452,8 @@ static std::unordered_map {"file_checksum_gen_factory", OptionTypeInfo::AsCustomSharedPtr( offsetof(struct ImmutableDBOptions, file_checksum_gen_factory), - OptionVerificationType::kByName, OptionTypeFlags::kAllowNull)}, + OptionVerificationType::kByNameAllowFromNull, + OptionTypeFlags::kAllowNull)}, {"statistics", OptionTypeInfo::AsCustomSharedPtr( // Statistics should not be compared and can be null @@ -529,19 +530,63 @@ const std::string OptionsHelper::kDBOptionsName = "DBOptions"; class MutableDBConfigurable : public Configurable { public: - explicit MutableDBConfigurable(const MutableDBOptions& mdb) { - mutable_ = mdb; + explicit MutableDBConfigurable( + const MutableDBOptions& mdb, + const std::unordered_map* map = nullptr) + : mutable_(mdb), opt_map_(map) { RegisterOptions(&mutable_, &db_mutable_options_type_info); } + bool OptionsAreEqual(const ConfigOptions& config_options, + const OptionTypeInfo& opt_info, + const std::string& opt_name, const void* const this_ptr, + const void* const that_ptr, + std::string* mismatch) const override { + bool equals = opt_info.AreEqual(config_options, opt_name, this_ptr, + that_ptr, mismatch); + if (!equals && opt_info.IsByName()) { + if (opt_map_ == nullptr) { + equals = true; + } else { + const auto& iter = opt_map_->find(opt_name); + if (iter == opt_map_->end()) { + equals = true; + } else { + equals = opt_info.AreEqualByName(config_options, opt_name, this_ptr, + iter->second); + } + } + if (equals) { // False alarm, clear mismatch + *mismatch = ""; + } + } + if (equals && opt_info.IsConfigurable() && opt_map_ != nullptr) { + const auto* this_config = opt_info.AsRawPointer(this_ptr); + if (this_config == nullptr) { + const auto& iter = opt_map_->find(opt_name); + // If the name exists in the map and is not empty/null, + // then the this_config should be set. + if (iter != opt_map_->end() && !iter->second.empty() && + iter->second != kNullptrString) { + *mismatch = opt_name; + equals = false; + } + } + } + return equals; + } + protected: MutableDBOptions mutable_; + const std::unordered_map* opt_map_; }; class DBOptionsConfigurable : public MutableDBConfigurable { public: - explicit DBOptionsConfigurable(const DBOptions& opts) - : MutableDBConfigurable(MutableDBOptions(opts)), db_options_(opts) { + explicit DBOptionsConfigurable( + const DBOptions& opts, + const std::unordered_map* map = nullptr) + : MutableDBConfigurable(MutableDBOptions(opts), map), db_options_(opts) { // The ImmutableDBOptions currently requires the env to be non-null. Make // sure it is if (opts.env != nullptr) { @@ -585,8 +630,10 @@ std::unique_ptr DBOptionsAsConfigurable( std::unique_ptr ptr(new MutableDBConfigurable(opts)); return ptr; } -std::unique_ptr DBOptionsAsConfigurable(const DBOptions& opts) { - std::unique_ptr ptr(new DBOptionsConfigurable(opts)); +std::unique_ptr DBOptionsAsConfigurable( + const DBOptions& opts, + const std::unordered_map* opt_map) { + std::unique_ptr ptr(new DBOptionsConfigurable(opts, opt_map)); return ptr; } #endif // ROCKSDB_LITE diff --git a/options/options_helper.h b/options/options_helper.h index 147f6b3ee..e3e9cea1f 100644 --- a/options/options_helper.h +++ b/options/options_helper.h @@ -47,7 +47,9 @@ void UpdateColumnFamilyOptions(const MutableCFOptions& moptions, #ifndef ROCKSDB_LITE std::unique_ptr DBOptionsAsConfigurable( const MutableDBOptions& opts); -std::unique_ptr DBOptionsAsConfigurable(const DBOptions& opts); +std::unique_ptr DBOptionsAsConfigurable( + const DBOptions& opts, + const std::unordered_map* opt_map = nullptr); std::unique_ptr CFOptionsAsConfigurable( const MutableCFOptions& opts); std::unique_ptr CFOptionsAsConfigurable( diff --git a/options/options_parser.cc b/options/options_parser.cc index 42cde218a..426e30013 100644 --- a/options/options_parser.cc +++ b/options/options_parser.cc @@ -553,6 +553,12 @@ Status RocksDBOptionsParser::VerifyRocksDBOptionsFromFile( ConfigOptions config_options = config_options_in; config_options.invoke_prepare_options = false; // No need to do a prepare for verify + if (config_options.sanity_level < ConfigOptions::kSanityLevelExactMatch) { + // If we are not doing an exact comparison, we should ignore + // unsupported options, as they may cause the Parse to fail + // (if the ObjectRegistry is not initialized) + config_options.ignore_unsupported_options = true; + } Status s = parser.Parse(config_options, file_name, fs); if (!s.ok()) { return s; @@ -622,9 +628,9 @@ Status RocksDBOptionsParser::VerifyRocksDBOptionsFromFile( Status RocksDBOptionsParser::VerifyDBOptions( const ConfigOptions& config_options, const DBOptions& base_opt, const DBOptions& file_opt, - const std::unordered_map* /*opt_map*/) { - auto base_config = DBOptionsAsConfigurable(base_opt); - auto file_config = DBOptionsAsConfigurable(file_opt); + const std::unordered_map* opt_map) { + auto base_config = DBOptionsAsConfigurable(base_opt, opt_map); + auto file_config = DBOptionsAsConfigurable(file_opt, opt_map); std::string mismatch; if (!base_config->AreEquivalent(config_options, file_config.get(), &mismatch)) { diff --git a/options/options_test.cc b/options/options_test.cc index 52b5ac7e1..e2b098a0d 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -3710,37 +3710,86 @@ TEST_F(OptionsParserTest, DifferentDefault) { ASSERT_EQ(5000, small_opts.max_open_files); } -class OptionsSanityCheckTest : public OptionsParserTest { +class OptionsSanityCheckTest : public OptionsParserTest, + public ::testing::WithParamInterface { + protected: + ConfigOptions config_options_; + public: - OptionsSanityCheckTest() {} + OptionsSanityCheckTest() { + config_options_.ignore_unknown_options = false; + config_options_.ignore_unsupported_options = GetParam(); + config_options_.input_strings_escaped = true; + } protected: - Status SanityCheckCFOptions(const ColumnFamilyOptions& cf_opts, - ConfigOptions::SanityLevel level, - bool input_strings_escaped = true) { - ConfigOptions config_options; - config_options.sanity_level = level; - config_options.ignore_unknown_options = false; - config_options.input_strings_escaped = input_strings_escaped; - + Status SanityCheckOptions(const DBOptions& db_opts, + const ColumnFamilyOptions& cf_opts, + ConfigOptions::SanityLevel level) { + config_options_.sanity_level = level; return RocksDBOptionsParser::VerifyRocksDBOptionsFromFile( - config_options, DBOptions(), {"default"}, {cf_opts}, kOptionsFileName, + config_options_, db_opts, {"default"}, {cf_opts}, kOptionsFileName, fs_.get()); } - Status PersistCFOptions(const ColumnFamilyOptions& cf_opts) { + Status SanityCheckCFOptions(const ColumnFamilyOptions& cf_opts, + ConfigOptions::SanityLevel level) { + return SanityCheckOptions(DBOptions(), cf_opts, level); + } + + void SanityCheckCFOptions(const ColumnFamilyOptions& opts, bool exact) { + ASSERT_OK(SanityCheckCFOptions( + opts, ConfigOptions::kSanityLevelLooselyCompatible)); + ASSERT_OK(SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelNone)); + if (exact) { + ASSERT_OK( + SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch)); + } else { + ASSERT_NOK( + SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch)); + } + } + + Status SanityCheckDBOptions(const DBOptions& db_opts, + ConfigOptions::SanityLevel level) { + return SanityCheckOptions(db_opts, ColumnFamilyOptions(), level); + } + + void SanityCheckDBOptions(const DBOptions& opts, bool exact) { + ASSERT_OK(SanityCheckDBOptions( + opts, ConfigOptions::kSanityLevelLooselyCompatible)); + ASSERT_OK(SanityCheckDBOptions(opts, ConfigOptions::kSanityLevelNone)); + if (exact) { + ASSERT_OK( + SanityCheckDBOptions(opts, ConfigOptions::kSanityLevelExactMatch)); + } else { + ASSERT_NOK( + SanityCheckDBOptions(opts, ConfigOptions::kSanityLevelExactMatch)); + } + } + + Status PersistOptions(const DBOptions& db_opts, + const ColumnFamilyOptions& cf_opts) { Status s = fs_->DeleteFile(kOptionsFileName, IOOptions(), nullptr); if (!s.ok()) { return s; } - return PersistRocksDBOptions(DBOptions(), {"default"}, {cf_opts}, + return PersistRocksDBOptions(db_opts, {"default"}, {cf_opts}, kOptionsFileName, fs_.get()); } + Status PersistCFOptions(const ColumnFamilyOptions& cf_opts) { + return PersistOptions(DBOptions(), cf_opts); + } + + Status PersistDBOptions(const DBOptions& db_opts) { + return PersistOptions(db_opts, ColumnFamilyOptions()); + } + const std::string kOptionsFileName = "OPTIONS"; }; -TEST_F(OptionsSanityCheckTest, SanityCheck) { +TEST_P(OptionsSanityCheckTest, CFOptionsSanityCheck) { ColumnFamilyOptions opts; Random rnd(301); @@ -3792,11 +3841,7 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) { opts.prefix_extractor.reset(NewFixedPrefixTransform(15)); // expect pass only in // ConfigOptions::kSanityLevelLooselyCompatible - ASSERT_NOK( - SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch)); - ASSERT_OK(SanityCheckCFOptions( - opts, ConfigOptions::kSanityLevelLooselyCompatible)); - ASSERT_OK(SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelNone)); + SanityCheckCFOptions(opts, false); // Change prefix extractor from non-nullptr to nullptr opts.prefix_extractor.reset(); @@ -3836,8 +3881,7 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) { // persist the change ASSERT_OK(PersistCFOptions(opts)); - ASSERT_OK( - SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch)); + SanityCheckCFOptions(opts, config_options_.ignore_unsupported_options); for (int test = 0; test < 5; ++test) { // change the merge operator @@ -3848,8 +3892,7 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) { // persist the change ASSERT_OK(PersistCFOptions(opts)); - ASSERT_OK( - SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch)); + SanityCheckCFOptions(opts, config_options_.ignore_unsupported_options); } // Test when going from merge operator -> nullptr @@ -3860,8 +3903,7 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) { // persist the change ASSERT_OK(PersistCFOptions(opts)); - ASSERT_OK( - SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch)); + SanityCheckCFOptions(opts, true); } // compaction_filter @@ -3869,15 +3911,11 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) { for (int test = 0; test < 5; ++test) { // change the compaction filter opts.compaction_filter = test::RandomCompactionFilter(&rnd); - ASSERT_NOK( - SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch)); - ASSERT_OK(SanityCheckCFOptions( - opts, ConfigOptions::kSanityLevelLooselyCompatible)); + SanityCheckCFOptions(opts, false); // persist the change ASSERT_OK(PersistCFOptions(opts)); - ASSERT_OK( - SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch)); + SanityCheckCFOptions(opts, config_options_.ignore_unsupported_options); delete opts.compaction_filter; opts.compaction_filter = nullptr; } @@ -3889,19 +3927,57 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) { // change the compaction filter factory opts.compaction_filter_factory.reset( test::RandomCompactionFilterFactory(&rnd)); - ASSERT_NOK( - SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch)); - ASSERT_OK(SanityCheckCFOptions( - opts, ConfigOptions::kSanityLevelLooselyCompatible)); + SanityCheckCFOptions(opts, false); // persist the change ASSERT_OK(PersistCFOptions(opts)); - ASSERT_OK( - SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch)); + SanityCheckCFOptions(opts, config_options_.ignore_unsupported_options); } } } +TEST_P(OptionsSanityCheckTest, DBOptionsSanityCheck) { + DBOptions opts; + Random rnd(301); + + // default DBOptions + { + ASSERT_OK(PersistDBOptions(opts)); + ASSERT_OK( + SanityCheckDBOptions(opts, ConfigOptions::kSanityLevelExactMatch)); + } + + // File checksum generator + { + class MockFileChecksumGenFactory : public FileChecksumGenFactory { + public: + static const char* kClassName() { return "Mock"; } + const char* Name() const override { return kClassName(); } + std::unique_ptr CreateFileChecksumGenerator( + const FileChecksumGenContext& /*context*/) override { + return nullptr; + } + }; + + // Okay to change file_checksum_gen_factory form nullptr to non-nullptr + ASSERT_EQ(opts.file_checksum_gen_factory.get(), nullptr); + opts.file_checksum_gen_factory.reset(new MockFileChecksumGenFactory()); + + // persist the change + ASSERT_OK(PersistDBOptions(opts)); + SanityCheckDBOptions(opts, config_options_.ignore_unsupported_options); + + // Change file_checksum_gen_factory from non-nullptr to nullptr + opts.file_checksum_gen_factory.reset(); + // expect pass as it's safe to change file_checksum_gen_factory + // from non-null to null + SanityCheckDBOptions(opts, false); + } + // persist the change + ASSERT_OK(PersistDBOptions(opts)); + ASSERT_OK(SanityCheckDBOptions(opts, ConfigOptions::kSanityLevelExactMatch)); +} + namespace { bool IsEscapedString(const std::string& str) { for (size_t i = 0; i < str.size(); ++i) { @@ -4612,7 +4688,11 @@ TEST_F(ConfigOptionsTest, MergeOperatorFromString) { ASSERT_NE(delimiter, nullptr); ASSERT_EQ(*delimiter, "&&"); } + +INSTANTIATE_TEST_CASE_P(OptionsSanityCheckTest, OptionsSanityCheckTest, + ::testing::Bool()); #endif // !ROCKSDB_LITE + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { diff --git a/util/slice.cc b/util/slice.cc index eb3f0e6a0..f96926cfc 100644 --- a/util/slice.cc +++ b/util/slice.cc @@ -218,49 +218,49 @@ Status SliceTransform::CreateFromString( value, &id, &opt_map); if (!status.ok()) { // GetOptionsMap failed return status; - } + } else if (id.empty() && opt_map.empty()) { + result->reset(); + } else { #ifndef ROCKSDB_LITE - status = config_options.registry->NewSharedObject(id, result); + status = config_options.registry->NewSharedObject(id, result); #else - auto Matches = [](const std::string& input, size_t size, const char* pattern, - char sep) { - auto plen = strlen(pattern); - return (size > plen + 2 && input[plen] == sep && - StartsWith(input, pattern)); - }; - - auto size = id.size(); - if (id == NoopTransform::kClassName()) { - result->reset(NewNoopTransform()); - } else if (Matches(id, size, FixedPrefixTransform::kNickName(), ':')) { - auto fixed = strlen(FixedPrefixTransform::kNickName()); - auto len = ParseSizeT(id.substr(fixed + 1)); - result->reset(NewFixedPrefixTransform(len)); - } else if (Matches(id, size, CappedPrefixTransform::kNickName(), ':')) { - auto capped = strlen(CappedPrefixTransform::kNickName()); - auto len = ParseSizeT(id.substr(capped + 1)); - result->reset(NewCappedPrefixTransform(len)); - } else if (Matches(id, size, CappedPrefixTransform::kClassName(), '.')) { - auto capped = strlen(CappedPrefixTransform::kClassName()); - auto len = ParseSizeT(id.substr(capped + 1)); - result->reset(NewCappedPrefixTransform(len)); - } else if (Matches(id, size, FixedPrefixTransform::kClassName(), '.')) { - auto fixed = strlen(FixedPrefixTransform::kClassName()); - auto len = ParseSizeT(id.substr(fixed + 1)); - result->reset(NewFixedPrefixTransform(len)); - } else { - status = Status::NotSupported("Cannot load object in LITE mode ", id); - } + auto Matches = [](const std::string& input, size_t size, + const char* pattern, char sep) { + auto plen = strlen(pattern); + return (size > plen + 2 && input[plen] == sep && + StartsWith(input, pattern)); + }; + + auto size = id.size(); + if (id == NoopTransform::kClassName()) { + result->reset(NewNoopTransform()); + } else if (Matches(id, size, FixedPrefixTransform::kNickName(), ':')) { + auto fixed = strlen(FixedPrefixTransform::kNickName()); + auto len = ParseSizeT(id.substr(fixed + 1)); + result->reset(NewFixedPrefixTransform(len)); + } else if (Matches(id, size, CappedPrefixTransform::kNickName(), ':')) { + auto capped = strlen(CappedPrefixTransform::kNickName()); + auto len = ParseSizeT(id.substr(capped + 1)); + result->reset(NewCappedPrefixTransform(len)); + } else if (Matches(id, size, CappedPrefixTransform::kClassName(), '.')) { + auto capped = strlen(CappedPrefixTransform::kClassName()); + auto len = ParseSizeT(id.substr(capped + 1)); + result->reset(NewCappedPrefixTransform(len)); + } else if (Matches(id, size, FixedPrefixTransform::kClassName(), '.')) { + auto fixed = strlen(FixedPrefixTransform::kClassName()); + auto len = ParseSizeT(id.substr(fixed + 1)); + result->reset(NewFixedPrefixTransform(len)); + } else { + status = Status::NotSupported("Cannot load object in LITE mode ", id); + } #endif // ROCKSDB_LITE - if (!status.ok()) { if (config_options.ignore_unsupported_options && status.IsNotSupported()) { return Status::OK(); - } else { - return status; + } else if (status.ok()) { + SliceTransform* transform = const_cast(result->get()); + status = + Customizable::ConfigureNewObject(config_options, transform, opt_map); } - } else if (result->get() != nullptr) { - SliceTransform* transform = const_cast(result->get()); - status = transform->ConfigureFromMap(config_options, opt_map); } return status; } // namespace ROCKSDB_NAMESPACE -- GitLab