From 7aa31ba4a99d8e0443a3b101c0c3cdc3fd54c0ba Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 30 Nov 2021 13:22:27 -0800 Subject: [PATCH] Fix GetOptionsPtr for Wrapped Customizable; Allow null options map (#9213) Summary: 1. Fix GetOptionsPtr for Wrapped (Inner() != nullptr) Customizable objects. This allows the inner options to be returned via this method. 2. Allow the option type map to be nullptr. This allows objects to be registered as options (for GetOptionsPtr) but not be used by the configuration methods. Added tests as appropriate. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9213 Reviewed By: zhichao-cao Differential Revision: D32718882 Pulled By: mrambacher fbshipit-source-id: 563203d1f006a2629060feb31c5dff9a233e1e83 --- include/rocksdb/customizable.h | 14 +++ options/configurable.cc | 166 ++++++++++++++++++--------------- options/configurable_test.cc | 24 +++++ options/customizable_test.cc | 82 ++++++++++++---- 4 files changed, 191 insertions(+), 95 deletions(-) diff --git a/include/rocksdb/customizable.h b/include/rocksdb/customizable.h index d6bd07087..0a018eace 100644 --- a/include/rocksdb/customizable.h +++ b/include/rocksdb/customizable.h @@ -101,6 +101,20 @@ class Customizable : public Configurable { } } + const void* GetOptionsPtr(const std::string& name) const override { + const void* ptr = Configurable::GetOptionsPtr(name); + if (ptr != nullptr) { + return ptr; + } else { + const auto inner = Inner(); + if (inner != nullptr) { + return inner->GetOptionsPtr(name); + } else { + return nullptr; + } + } + } + // Returns the named instance of the Customizable as a T*, or nullptr if not // found. This method uses IsInstanceOf/Inner to find the appropriate class // instance and then casts it to the expected return type. diff --git a/options/configurable.cc b/options/configurable.cc index a98d2646b..3ce3c0ec8 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -43,21 +43,23 @@ Status Configurable::PrepareOptions(const ConfigOptions& opts) { Status status = Status::OK(); #ifndef ROCKSDB_LITE for (auto opt_iter : options_) { - for (auto map_iter : *(opt_iter.type_map)) { - auto& opt_info = map_iter.second; - if (!opt_info.IsDeprecated() && !opt_info.IsAlias() && - opt_info.IsConfigurable()) { - if (!opt_info.IsEnabled(OptionTypeFlags::kDontPrepare)) { - Configurable* config = - opt_info.AsRawPointer(opt_iter.opt_ptr); - if (config != nullptr) { - status = config->PrepareOptions(opts); - if (!status.ok()) { - return status; + if (opt_iter.type_map != nullptr) { + for (auto map_iter : *(opt_iter.type_map)) { + auto& opt_info = map_iter.second; + if (!opt_info.IsDeprecated() && !opt_info.IsAlias() && + opt_info.IsConfigurable()) { + if (!opt_info.IsEnabled(OptionTypeFlags::kDontPrepare)) { + Configurable* config = + opt_info.AsRawPointer(opt_iter.opt_ptr); + if (config != nullptr) { + status = config->PrepareOptions(opts); + if (!status.ok()) { + return status; + } + } else if (!opt_info.CanBeNull()) { + status = Status::NotFound("Missing configurable object", + map_iter.first); } - } else if (!opt_info.CanBeNull()) { - status = - Status::NotFound("Missing configurable object", map_iter.first); } } } @@ -74,20 +76,22 @@ Status Configurable::ValidateOptions(const DBOptions& db_opts, Status status; #ifndef ROCKSDB_LITE for (auto opt_iter : options_) { - for (auto map_iter : *(opt_iter.type_map)) { - auto& opt_info = map_iter.second; - if (!opt_info.IsDeprecated() && !opt_info.IsAlias()) { - if (opt_info.IsConfigurable()) { - const Configurable* config = - opt_info.AsRawPointer(opt_iter.opt_ptr); - if (config != nullptr) { - status = config->ValidateOptions(db_opts, cf_opts); - } else if (!opt_info.CanBeNull()) { - status = - Status::NotFound("Missing configurable object", map_iter.first); - } - if (!status.ok()) { - return status; + if (opt_iter.type_map != nullptr) { + for (auto map_iter : *(opt_iter.type_map)) { + auto& opt_info = map_iter.second; + if (!opt_info.IsDeprecated() && !opt_info.IsAlias()) { + if (opt_info.IsConfigurable()) { + const Configurable* config = + opt_info.AsRawPointer(opt_iter.opt_ptr); + if (config != nullptr) { + status = config->ValidateOptions(db_opts, cf_opts); + } else if (!opt_info.CanBeNull()) { + status = Status::NotFound("Missing configurable object", + map_iter.first); + } + if (!status.ok()) { + return status; + } } } } @@ -124,11 +128,13 @@ const OptionTypeInfo* ConfigurableHelper::FindOption( const std::vector& options, const std::string& short_name, std::string* opt_name, void** opt_ptr) { for (auto iter : options) { - const auto opt_info = - OptionTypeInfo::Find(short_name, *(iter.type_map), opt_name); - if (opt_info != nullptr) { - *opt_ptr = iter.opt_ptr; - return opt_info; + if (iter.type_map != nullptr) { + const auto opt_info = + OptionTypeInfo::Find(short_name, *(iter.type_map), opt_name); + if (opt_info != nullptr) { + *opt_ptr = iter.opt_ptr; + return opt_info; + } } } return nullptr; @@ -280,12 +286,14 @@ Status ConfigurableHelper::ConfigureOptions( if (!opts_map.empty()) { #ifndef ROCKSDB_LITE for (const auto& iter : configurable.options_) { - s = ConfigureSomeOptions(config_options, configurable, *(iter.type_map), - &remaining, iter.opt_ptr); - if (remaining.empty()) { // Are there more options left? - break; - } else if (!s.ok()) { - break; + if (iter.type_map != nullptr) { + s = ConfigureSomeOptions(config_options, configurable, *(iter.type_map), + &remaining, iter.opt_ptr); + if (remaining.empty()) { // Are there more options left? + break; + } else if (!s.ok()) { + break; + } } } #else @@ -573,36 +581,38 @@ Status ConfigurableHelper::SerializeOptions(const ConfigOptions& config_options, std::string* result) { assert(result); for (auto const& opt_iter : configurable.options_) { - for (const auto& map_iter : *(opt_iter.type_map)) { - const auto& opt_name = map_iter.first; - const auto& opt_info = map_iter.second; - if (opt_info.ShouldSerialize()) { - std::string value; - Status s; - if (!config_options.mutable_options_only) { - s = opt_info.Serialize(config_options, prefix + opt_name, - opt_iter.opt_ptr, &value); - } else if (opt_info.IsMutable()) { - ConfigOptions copy = config_options; - copy.mutable_options_only = false; - s = opt_info.Serialize(copy, prefix + opt_name, opt_iter.opt_ptr, - &value); - } else if (opt_info.IsConfigurable()) { - // If it is a Configurable and we are either printing all of the - // details or not printing only the name, this option should be - // included in the list - if (config_options.IsDetailed() || - !opt_info.IsEnabled(OptionTypeFlags::kStringNameOnly)) { + if (opt_iter.type_map != nullptr) { + for (const auto& map_iter : *(opt_iter.type_map)) { + const auto& opt_name = map_iter.first; + const auto& opt_info = map_iter.second; + if (opt_info.ShouldSerialize()) { + std::string value; + Status s; + if (!config_options.mutable_options_only) { s = opt_info.Serialize(config_options, prefix + opt_name, opt_iter.opt_ptr, &value); + } else if (opt_info.IsMutable()) { + ConfigOptions copy = config_options; + copy.mutable_options_only = false; + s = opt_info.Serialize(copy, prefix + opt_name, opt_iter.opt_ptr, + &value); + } else if (opt_info.IsConfigurable()) { + // If it is a Configurable and we are either printing all of the + // details or not printing only the name, this option should be + // included in the list + if (config_options.IsDetailed() || + !opt_info.IsEnabled(OptionTypeFlags::kStringNameOnly)) { + s = opt_info.Serialize(config_options, prefix + opt_name, + opt_iter.opt_ptr, &value); + } + } + if (!s.ok()) { + return s; + } else if (!value.empty()) { + // = + result->append(prefix + opt_name + "=" + value + + config_options.delimiter); } - } - if (!s.ok()) { - return s; - } else if (!value.empty()) { - // = - result->append(prefix + opt_name + "=" + value + - config_options.delimiter); } } } @@ -629,16 +639,18 @@ Status ConfigurableHelper::ListOptions( const std::string& prefix, std::unordered_set* result) { Status status; for (auto const& opt_iter : configurable.options_) { - for (const auto& map_iter : *(opt_iter.type_map)) { - const auto& opt_name = map_iter.first; - const auto& opt_info = map_iter.second; - // If the option is no longer used in rocksdb and marked as deprecated, - // we skip it in the serialization. - if (!opt_info.IsDeprecated() && !opt_info.IsAlias()) { - if (!config_options.mutable_options_only) { - result->emplace(prefix + opt_name); - } else if (opt_info.IsMutable()) { - result->emplace(prefix + opt_name); + if (opt_iter.type_map != nullptr) { + for (const auto& map_iter : *(opt_iter.type_map)) { + const auto& opt_name = map_iter.first; + const auto& opt_info = map_iter.second; + // If the option is no longer used in rocksdb and marked as deprecated, + // we skip it in the serialization. + if (!opt_info.IsDeprecated() && !opt_info.IsAlias()) { + if (!config_options.mutable_options_only) { + result->emplace(prefix + opt_name); + } else if (opt_info.IsMutable()) { + result->emplace(prefix + opt_name); + } } } } @@ -702,7 +714,7 @@ bool ConfigurableHelper::AreEquivalent(const ConfigOptions& config_options, if (this_offset != that_offset) { if (this_offset == nullptr || that_offset == nullptr) { return false; - } else { + } else if (o.type_map != nullptr) { for (const auto& map_iter : *(o.type_map)) { const auto& opt_info = map_iter.second; if (config_options.IsCheckEnabled(opt_info.GetSanityLevel())) { diff --git a/options/configurable_test.cc b/options/configurable_test.cc index fd412528f..6cc83b847 100644 --- a/options/configurable_test.cc +++ b/options/configurable_test.cc @@ -656,6 +656,30 @@ TEST_F(ConfigurableTest, TestNoCompare) { ASSERT_TRUE(base->AreEquivalent(config_options_, copy.get(), &mismatch)); ASSERT_FALSE(copy->AreEquivalent(config_options_, base.get(), &mismatch)); } + +TEST_F(ConfigurableTest, NullOptionMapTest) { + std::unique_ptr base; + std::unordered_set names; + std::string str; + + base.reset( + SimpleConfigurable::Create("c", TestConfigMode::kDefaultMode, nullptr)); + ASSERT_NOK(base->ConfigureFromString(config_options_, "int=10")); + ASSERT_NOK(base->ConfigureFromString(config_options_, "int=20")); + ASSERT_NOK(base->ConfigureOption(config_options_, "int", "20")); + ASSERT_NOK(base->GetOption(config_options_, "int", &str)); + ASSERT_NE(base->GetOptions("c"), nullptr); + ASSERT_OK(base->GetOptionNames(config_options_, &names)); + ASSERT_EQ(names.size(), 0UL); + ASSERT_OK(base->PrepareOptions(config_options_)); + ASSERT_OK(base->ValidateOptions(DBOptions(), ColumnFamilyOptions())); + std::unique_ptr copy; + copy.reset( + SimpleConfigurable::Create("c", TestConfigMode::kDefaultMode, nullptr)); + ASSERT_OK(base->GetOptionString(config_options_, &str)); + ASSERT_OK(copy->ConfigureFromString(config_options_, str)); + ASSERT_TRUE(base->AreEquivalent(config_options_, copy.get(), &str)); +} #endif static std::unordered_map TestFactories = { diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 776393c8f..98e953e3e 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -612,11 +612,20 @@ static std::unordered_map inner_option_info = { #endif // ROCKSDB_LITE }; +struct InnerOptions { + static const char* kName() { return "InnerOptions"; } + std::shared_ptr inner; +}; + class InnerCustomizable : public Customizable { public: - explicit InnerCustomizable(const std::shared_ptr& w) - : inner_(w) {} + explicit InnerCustomizable(const std::shared_ptr& w) { + iopts_.inner = w; + RegisterOptions(&iopts_, &inner_option_info); + } static const char* kClassName() { return "Inner"; } + const char* Name() const override { return kClassName(); } + bool IsInstanceOf(const std::string& name) const override { if (name == kClassName()) { return true; @@ -626,26 +635,51 @@ class InnerCustomizable : public Customizable { } protected: - const Customizable* Inner() const override { return inner_.get(); } + const Customizable* Inner() const override { return iopts_.inner.get(); } private: - std::shared_ptr inner_; + InnerOptions iopts_; +}; + +struct WrappedOptions1 { + static const char* kName() { return "WrappedOptions1"; } + int i = 42; }; class WrappedCustomizable1 : public InnerCustomizable { public: explicit WrappedCustomizable1(const std::shared_ptr& w) - : InnerCustomizable(w) {} + : InnerCustomizable(w) { + RegisterOptions(&wopts_, nullptr); + } const char* Name() const override { return kClassName(); } static const char* kClassName() { return "Wrapped1"; } + + private: + WrappedOptions1 wopts_; }; +struct WrappedOptions2 { + static const char* kName() { return "WrappedOptions2"; } + std::string s = "42"; +}; class WrappedCustomizable2 : public InnerCustomizable { public: explicit WrappedCustomizable2(const std::shared_ptr& w) : InnerCustomizable(w) {} + const void* GetOptionsPtr(const std::string& name) const override { + if (name == WrappedOptions2::kName()) { + return &wopts_; + } else { + return InnerCustomizable::GetOptionsPtr(name); + } + } + const char* Name() const override { return kClassName(); } static const char* kClassName() { return "Wrapped2"; } + + private: + WrappedOptions2 wopts_; }; } // namespace @@ -677,6 +711,29 @@ TEST_F(CustomizableTest, WrappedInnerTest) { ASSERT_EQ(wc2->CheckedCast(), ac.get()); } +TEST_F(CustomizableTest, CustomizableInnerTest) { + std::shared_ptr c = + std::make_shared(std::make_shared("a")); + std::shared_ptr wc1 = std::make_shared(c); + std::shared_ptr wc2 = std::make_shared(c); + auto inner = c->GetOptions(); + ASSERT_NE(inner, nullptr); + + auto aopts = c->GetOptions(); + ASSERT_NE(aopts, nullptr); + ASSERT_EQ(aopts, wc1->GetOptions()); + ASSERT_EQ(aopts, wc2->GetOptions()); + auto w1opts = wc1->GetOptions(); + ASSERT_NE(w1opts, nullptr); + ASSERT_EQ(c->GetOptions(), nullptr); + ASSERT_EQ(wc2->GetOptions(), nullptr); + + auto w2opts = wc2->GetOptions(); + ASSERT_NE(w2opts, nullptr); + ASSERT_EQ(c->GetOptions(), nullptr); + ASSERT_EQ(wc1->GetOptions(), nullptr); +} + TEST_F(CustomizableTest, CopyObjectTest) { class CopyCustomizable : public Customizable { public: @@ -714,20 +771,9 @@ TEST_F(CustomizableTest, CopyObjectTest) { } TEST_F(CustomizableTest, TestStringDepth) { - class ShallowCustomizable : public Customizable { - public: - ShallowCustomizable() { - inner_ = std::make_shared("a"); - RegisterOptions("inner", &inner_, &inner_option_info); - } - static const char* kClassName() { return "shallow"; } - const char* Name() const override { return kClassName(); } - - private: - std::shared_ptr inner_; - }; ConfigOptions shallow = config_options_; - std::unique_ptr c(new ShallowCustomizable()); + std::unique_ptr c( + new InnerCustomizable(std::make_shared("a"))); std::string opt_str; shallow.depth = ConfigOptions::Depth::kDepthShallow; ASSERT_OK(c->GetOptionString(shallow, &opt_str));