diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 806a368a6c17e857bea53e9f6f3fe9dcffe0172f..4ca04beff9b9c597239da2eb6d81c62dc995f6b2 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 7a91c9e6d5fc14958e056f34362d3e3b602d903f..d99df242cf425a56f8d7d6d0b12eafb38a876b76 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 147f6b3ee14241a774b0c9b92269871a7b414182..e3e9cea1f2791feb42f1075c8363d04befc4c210 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 42cde218aad694dfb8befc1931c4cd4abfd10b15..426e300137f17c8d184a4bc607e5357309e4a46a 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 52b5ac7e161baf7cf064490c02b3074ce9d634ec..e2b098a0d75239c040377871367e2f43c67c1682 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 eb3f0e6a02f65e2ee65dfc5ac1ea51f8ec622de6..f96926cfc42095bce2db3c90532a8489405a0c00 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