From 41c4b665f4c6c9ec17be7b75e1be63c13a3af1ab Mon Sep 17 00:00:00 2001 From: mrambacher Date: Wed, 30 Jun 2021 14:08:19 -0700 Subject: [PATCH] Fix PrepareOptions for Customizable Classes (#8468) Summary: Added the Customizable::ConfigureNewObject method. The method will configure the object if options are found and invoke PrepareOptions if the flag is set properly. Added tests to test that PrepareOptions is properly called and to test if PrepareOptions fails. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8468 Reviewed By: zhichao-cao Differential Revision: D29494703 Pulled By: mrambacher fbshipit-source-id: d5767dee5d7a98620ac66190262101cd0aa9d2b7 --- include/rocksdb/customizable.h | 11 ++ include/rocksdb/utilities/customizable_util.h | 40 ++--- options/configurable.cc | 65 ++++---- options/customizable.cc | 12 ++ options/customizable_test.cc | 141 +++++++++++++++--- 5 files changed, 192 insertions(+), 77 deletions(-) diff --git a/include/rocksdb/customizable.h b/include/rocksdb/customizable.h index 48147e1f6..c04e71360 100644 --- a/include/rocksdb/customizable.h +++ b/include/rocksdb/customizable.h @@ -162,6 +162,17 @@ class Customizable : public Configurable { const std::string& opt_value, std::string* id, std::unordered_map* options); + // Helper method to configure a new object with the supplied options. + // If the object is not null and invoke_prepare_options=true, the object + // will be configured and prepared. + // Returns success if the object is properly configured and (optionally) + // prepared Returns InvalidArgument if the object is nullptr and there are + // options in the map Returns the result of the ConfigureFromMap or + // PrepareOptions + static Status ConfigureNewObject( + const ConfigOptions& config_options, Customizable* object, + const std::unordered_map& options); + // Returns the inner class when a Customizable implements a has-a (wrapped) // relationship. Derived classes that implement a has-a must override this // method in order to get CheckedCast to function properly. diff --git a/include/rocksdb/utilities/customizable_util.h b/include/rocksdb/utilities/customizable_util.h index 447873fae..7d629218e 100644 --- a/include/rocksdb/utilities/customizable_util.h +++ b/include/rocksdb/utilities/customizable_util.h @@ -71,12 +71,11 @@ static Status NewSharedObject( } else { status = Status::NotSupported("Cannot reset object "); } - if (!status.ok() || opt_map.empty()) { + if (!status.ok()) { return status; - } else if (result->get() != nullptr) { - return result->get()->ConfigureFromMap(config_options, opt_map); } else { - return Status::InvalidArgument("Cannot configure null object ", id); + return Customizable::ConfigureNewObject(config_options, result->get(), + opt_map); } } @@ -126,12 +125,9 @@ static Status LoadSharedObject(const ConfigOptions& config_options, } else { return NewSharedObject(config_options, id, opt_map, result); } - } else if (opt_map.empty()) { - return status; - } else if (result->get() != nullptr) { - return result->get()->ConfigureFromMap(config_options, opt_map); } else { - return Status::InvalidArgument("Cannot configure null object "); + return Customizable::ConfigureNewObject(config_options, result->get(), + opt_map); } } @@ -166,12 +162,11 @@ static Status NewUniqueObject( return Status::OK(); } } - if (!status.ok() || opt_map.empty()) { + if (!status.ok()) { return status; - } else if (result->get() != nullptr) { - return result->get()->ConfigureFromMap(config_options, opt_map); } else { - return Status::InvalidArgument("Cannot configure null object "); + return Customizable::ConfigureNewObject(config_options, result->get(), + opt_map); } } @@ -205,12 +200,9 @@ static Status LoadUniqueObject(const ConfigOptions& config_options, } else { return NewUniqueObject(config_options, id, opt_map, result); } - } else if (opt_map.empty()) { - return status; - } else if (result->get() != nullptr) { - return result->get()->ConfigureFromMap(config_options, opt_map); } else { - return Status::InvalidArgument("Cannot configure null object "); + return Customizable::ConfigureNewObject(config_options, result->get(), + opt_map); } } @@ -244,12 +236,10 @@ static Status NewStaticObject( return Status::OK(); } } - if (!status.ok() || opt_map.empty()) { + if (!status.ok()) { return status; - } else if (*result != nullptr) { - return (*result)->ConfigureFromMap(config_options, opt_map); } else { - return Status::InvalidArgument("Cannot configure null object "); + return Customizable::ConfigureNewObject(config_options, *result, opt_map); } } @@ -282,12 +272,8 @@ static Status LoadStaticObject(const ConfigOptions& config_options, } else { return NewStaticObject(config_options, id, opt_map, result); } - } else if (opt_map.empty()) { - return status; - } else if (*result != nullptr) { - return (*result)->ConfigureFromMap(config_options, opt_map); } else { - return Status::InvalidArgument("Cannot configure null object "); + return Customizable::ConfigureNewObject(config_options, *result, opt_map); } } } // namespace ROCKSDB_NAMESPACE diff --git a/options/configurable.cc b/options/configurable.cc index 4444a408f..4d0638b37 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -39,30 +39,33 @@ void Configurable::RegisterOptions( Status Configurable::PrepareOptions(const ConfigOptions& opts) { Status status = Status::OK(); + if (opts.invoke_prepare_options) { #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; + 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; + } + } else if (!opt_info.CanBeNull()) { + status = Status::NotFound("Missing configurable object", + map_iter.first); } } } } } - } -#else - (void)opts; #endif // ROCKSDB_LITE - if (status.ok()) { - prepared_ = true; + if (status.ok()) { + prepared_ = true; + } } return status; } @@ -158,17 +161,26 @@ Status Configurable::ConfigureOptions( const std::unordered_map& opts_map, std::unordered_map* unused) { std::string curr_opts; -#ifndef ROCKSDB_LITE - if (!config_options.ignore_unknown_options) { - // If we are not ignoring unused, get the defaults in case we need to reset + Status s; + if (!opts_map.empty()) { + // There are options in the map. + // Save the current configuration in curr_opts and then configure the + // options, but do not prepare them now. We will do all the prepare when + // the configuration is complete. ConfigOptions copy = config_options; - copy.depth = ConfigOptions::kDepthDetailed; - copy.delimiter = "; "; - GetOptionString(copy, &curr_opts).PermitUncheckedError(); - } + copy.invoke_prepare_options = false; +#ifndef ROCKSDB_LITE + if (!config_options.ignore_unknown_options) { + // If we are not ignoring unused, get the defaults in case we need to + // reset + copy.depth = ConfigOptions::kDepthDetailed; + copy.delimiter = "; "; + GetOptionString(copy, &curr_opts).PermitUncheckedError(); + } #endif // ROCKSDB_LITE - Status s = ConfigurableHelper::ConfigureOptions(config_options, *this, - opts_map, unused); + + s = ConfigurableHelper::ConfigureOptions(copy, *this, opts_map, unused); + } if (config_options.invoke_prepare_options && s.ok()) { s = PrepareOptions(config_options); } @@ -177,6 +189,7 @@ Status Configurable::ConfigureOptions( ConfigOptions reset = config_options; reset.ignore_unknown_options = true; reset.invoke_prepare_options = true; + reset.ignore_unsupported_options = true; // There are some options to reset from this current error ConfigureFromString(reset, curr_opts).PermitUncheckedError(); } diff --git a/options/customizable.cc b/options/customizable.cc index 2c599e138..754eff75c 100644 --- a/options/customizable.cc +++ b/options/customizable.cc @@ -104,4 +104,16 @@ Status Customizable::GetOptionsMap( return ConfigurableHelper::GetOptionsMap(value, "", id, props); } } + +Status Customizable::ConfigureNewObject( + const ConfigOptions& config_options, Customizable* object, + const std::unordered_map& opt_map) { + Status status; + if (object != nullptr) { + status = object->ConfigureFromMap(config_options, opt_map); + } else if (!opt_map.empty()) { + status = Status::InvalidArgument("Cannot configure null object "); + } + return status; +} } // namespace ROCKSDB_NAMESPACE diff --git a/options/customizable_test.cc b/options/customizable_test.cc index ea8dd51f7..04f5761fa 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -217,8 +217,8 @@ const FactoryFunc& s_func = #endif // ROCKSDB_LITE struct SimpleOptions { + static const char* kName() { return "simple"; } bool b = true; - bool is_mutable = true; std::unique_ptr cu; std::shared_ptr cs; TestCustomizable* cp = nullptr; @@ -246,28 +246,18 @@ class SimpleConfigurable : public Configurable { SimpleOptions simple_; public: - SimpleConfigurable() { - RegisterOptions("simple", &simple_, &simple_option_info); - } + SimpleConfigurable() { RegisterOptions(&simple_, &simple_option_info); } explicit SimpleConfigurable( const std::unordered_map* map) { - RegisterOptions("simple", &simple_, map); - } - - bool IsPrepared() const override { - if (simple_.is_mutable) { - return false; - } else { - return Configurable::IsPrepared(); - } + RegisterOptions(&simple_, map); } - - private: }; class CustomizableTest : public testing::Test { public: + CustomizableTest() { config_options_.invoke_prepare_options = false; } + ConfigOptions config_options_; }; @@ -285,7 +275,7 @@ TEST_F(CustomizableTest, CreateByNameTest) { return guard->get(); }); std::unique_ptr configurable(new SimpleConfigurable()); - SimpleOptions* simple = configurable->GetOptions("simple"); + SimpleOptions* simple = configurable->GetOptions(); ASSERT_NE(simple, nullptr); ASSERT_OK( configurable->ConfigureFromString(config_options_, "unique={id=TEST_1}")); @@ -313,7 +303,7 @@ TEST_F(CustomizableTest, SimpleConfigureTest) { }; std::unique_ptr configurable(new SimpleConfigurable()); ASSERT_OK(configurable->ConfigureFromMap(config_options_, opt_map)); - SimpleOptions* simple = configurable->GetOptions("simple"); + SimpleOptions* simple = configurable->GetOptions(); ASSERT_NE(simple, nullptr); ASSERT_NE(simple->cu, nullptr); ASSERT_EQ(simple->cu->GetId(), "A"); @@ -349,7 +339,7 @@ TEST_F(CustomizableTest, ConfigureFromPropsTest) { }; std::unique_ptr configurable(new SimpleConfigurable()); ASSERT_OK(configurable->ConfigureFromMap(config_options_, opt_map)); - SimpleOptions* simple = configurable->GetOptions("simple"); + SimpleOptions* simple = configurable->GetOptions(); ASSERT_NE(simple, nullptr); ASSERT_NE(simple->cu, nullptr); ASSERT_EQ(simple->cu->GetId(), "A"); @@ -372,7 +362,7 @@ TEST_F(CustomizableTest, ConfigureFromShortTest) { }; std::unique_ptr configurable(new SimpleConfigurable()); ASSERT_OK(configurable->ConfigureFromMap(config_options_, opt_map)); - SimpleOptions* simple = configurable->GetOptions("simple"); + SimpleOptions* simple = configurable->GetOptions(); ASSERT_NE(simple, nullptr); ASSERT_NE(simple->cu, nullptr); ASSERT_EQ(simple->cu->GetId(), "A"); @@ -385,13 +375,12 @@ TEST_F(CustomizableTest, AreEquivalentOptionsTest) { }; std::string mismatch; ConfigOptions config_options = config_options_; - config_options.invoke_prepare_options = false; std::unique_ptr c1(new SimpleConfigurable()); std::unique_ptr c2(new SimpleConfigurable()); ASSERT_OK(c1->ConfigureFromMap(config_options, opt_map)); ASSERT_OK(c2->ConfigureFromMap(config_options, opt_map)); ASSERT_TRUE(c1->AreEquivalent(config_options, c2.get(), &mismatch)); - SimpleOptions* simple = c1->GetOptions("simple"); + SimpleOptions* simple = c1->GetOptions(); ASSERT_TRUE( simple->cu->AreEquivalent(config_options, simple->cs.get(), &mismatch)); ASSERT_OK(simple->cu->ConfigureOption(config_options, "int", "2")); @@ -462,7 +451,7 @@ TEST_F(CustomizableTest, UniqueIdTest) { std::unique_ptr base(new SimpleConfigurable()); ASSERT_OK(base->ConfigureFromString(config_options_, "unique={id=A_1;int=1;bool=true}")); - SimpleOptions* simple = base->GetOptions("simple"); + SimpleOptions* simple = base->GetOptions(); ASSERT_NE(simple, nullptr); ASSERT_NE(simple->cu, nullptr); ASSERT_EQ(simple->cu->GetId(), std::string("A_1")); @@ -497,6 +486,110 @@ TEST_F(CustomizableTest, IsInstanceOfTest) { ASSERT_EQ(tc->CheckedCast(), nullptr); } +TEST_F(CustomizableTest, PrepareOptionsTest) { + static std::unordered_map p_option_info = { +#ifndef ROCKSDB_LITE + {"can_prepare", + {0, OptionType::kBoolean, OptionVerificationType::kNormal, + OptionTypeFlags::kNone}}, +#endif // ROCKSDB_LITE + }; + + class PrepareCustomizable : public TestCustomizable { + public: + bool can_prepare_ = true; + + PrepareCustomizable() : TestCustomizable("P") { + RegisterOptions("Prepare", &can_prepare_, &p_option_info); + } + + Status PrepareOptions(const ConfigOptions& opts) override { + if (!can_prepare_) { + return Status::InvalidArgument("Cannot Prepare"); + } else { + return TestCustomizable::PrepareOptions(opts); + } + } + }; + + ObjectLibrary::Default()->Register( + "P", + [](const std::string& /*name*/, std::unique_ptr* guard, + std::string* /* msg */) { + guard->reset(new PrepareCustomizable()); + return guard->get(); + }); + + std::unique_ptr base(new SimpleConfigurable()); + ConfigOptions prepared(config_options_); + prepared.invoke_prepare_options = true; + + ASSERT_FALSE(base->IsPrepared()); + ASSERT_OK(base->ConfigureFromString( + prepared, "unique=A_1; shared={id=B;string=s}; pointer.id=S")); + SimpleOptions* simple = base->GetOptions(); + ASSERT_NE(simple, nullptr); + ASSERT_NE(simple->cu, nullptr); + ASSERT_NE(simple->cs, nullptr); + ASSERT_NE(simple->cp, nullptr); + ASSERT_TRUE(base->IsPrepared()); + ASSERT_TRUE(simple->cu->IsPrepared()); + ASSERT_TRUE(simple->cs->IsPrepared()); + ASSERT_TRUE(simple->cp->IsPrepared()); + delete simple->cp; + base.reset(new SimpleConfigurable()); + ASSERT_OK(base->ConfigureFromString( + config_options_, "unique=A_1; shared={id=B;string=s}; pointer.id=S")); + + simple = base->GetOptions(); + ASSERT_NE(simple, nullptr); + ASSERT_NE(simple->cu, nullptr); + ASSERT_NE(simple->cs, nullptr); + ASSERT_NE(simple->cp, nullptr); + ASSERT_FALSE(base->IsPrepared()); + ASSERT_FALSE(simple->cu->IsPrepared()); + ASSERT_FALSE(simple->cs->IsPrepared()); + ASSERT_FALSE(simple->cp->IsPrepared()); + + ASSERT_OK(base->PrepareOptions(config_options_)); + ASSERT_FALSE(base->IsPrepared()); + ASSERT_FALSE(simple->cu->IsPrepared()); + ASSERT_FALSE(simple->cs->IsPrepared()); + ASSERT_FALSE(simple->cp->IsPrepared()); + ASSERT_OK(base->PrepareOptions(prepared)); + ASSERT_TRUE(base->IsPrepared()); + ASSERT_TRUE(simple->cu->IsPrepared()); + ASSERT_TRUE(simple->cs->IsPrepared()); + ASSERT_TRUE(simple->cp->IsPrepared()); + delete simple->cp; + base.reset(new SimpleConfigurable()); + + ASSERT_NOK( + base->ConfigureFromString(prepared, "unique={id=P; can_prepare=false}")); + simple = base->GetOptions(); + ASSERT_NE(simple, nullptr); + ASSERT_NE(simple->cu, nullptr); + ASSERT_FALSE(simple->cu->IsPrepared()); + + ASSERT_OK( + base->ConfigureFromString(prepared, "unique={id=P; can_prepare=true}")); + ASSERT_TRUE(simple->cu->IsPrepared()); + + ASSERT_OK(base->ConfigureFromString(config_options_, + "unique={id=P; can_prepare=true}")); + ASSERT_NE(simple->cu, nullptr); + ASSERT_FALSE(simple->cu->IsPrepared()); + ASSERT_OK(simple->cu->PrepareOptions(prepared)); + ASSERT_TRUE(simple->cu->IsPrepared()); + + ASSERT_OK(base->ConfigureFromString(config_options_, + "unique={id=P; can_prepare=false}")); + ASSERT_NE(simple->cu, nullptr); + ASSERT_FALSE(simple->cu->IsPrepared()); + ASSERT_NOK(simple->cu->PrepareOptions(prepared)); + ASSERT_FALSE(simple->cu->IsPrepared()); +} + static std::unordered_map inner_option_info = { #ifndef ROCKSDB_LITE {"inner", @@ -600,7 +693,7 @@ TEST_F(CustomizableTest, NewCustomizableTest) { A_count = 0; ASSERT_OK(base->ConfigureFromString(config_options_, "unique={id=A_1;int=1;bool=true}")); - SimpleOptions* simple = base->GetOptions("simple"); + SimpleOptions* simple = base->GetOptions(); ASSERT_NE(simple, nullptr); ASSERT_NE(simple->cu, nullptr); ASSERT_EQ(A_count, 1); // Created one A @@ -698,7 +791,7 @@ TEST_F(CustomizableTest, MutableOptionsTest) { static std::unordered_map immutable_option_info = {{"immutable", OptionTypeInfo::AsCustomSharedPtr( - 0, OptionVerificationType::kNormal, OptionTypeFlags::kNone)}}; + 0, OptionVerificationType::kNormal, OptionTypeFlags::kAllowNull)}}; class MutableCustomizable : public Customizable { private: