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: