From 6bab3a34e9348896ab3ac1a66ec53717d680512c Mon Sep 17 00:00:00 2001 From: mrambacher Date: Mon, 26 Apr 2021 03:12:35 -0700 Subject: [PATCH] Move RegisterOptions into the Configurable API (#8223) Summary: As previously coded, a Configurable extension would need access to code not in the public API. This change moves RegisterOptions into the Configurable class and therefore available to public extensions. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8223 Reviewed By: anand1976 Differential Revision: D27960188 Pulled By: mrambacher fbshipit-source-id: ac88b19397183df633902def5b5701b9b65fbf40 --- include/rocksdb/configurable.h | 29 +++++++++++++++++++ options/cf_options.cc | 8 ++--- options/configurable.cc | 8 ++--- options/configurable_helper.h | 29 ------------------- options/configurable_test.cc | 27 ++++++----------- options/configurable_test.h | 5 ++-- options/customizable_helper.h | 1 - options/customizable_test.cc | 25 +++++++--------- options/db_options.cc | 10 +++---- .../block_based/block_based_table_factory.cc | 3 +- table/cuckoo/cuckoo_table_factory.cc | 3 +- table/plain/plain_table_factory.cc | 3 +- 12 files changed, 65 insertions(+), 86 deletions(-) diff --git a/include/rocksdb/configurable.h b/include/rocksdb/configurable.h index 655b3b43d..b56072dbe 100644 --- a/include/rocksdb/configurable.h +++ b/include/rocksdb/configurable.h @@ -350,6 +350,35 @@ class Configurable { // Given a name (e.g. rocksdb.my.type.opt), returns the short name (opt) virtual std::string GetOptionName(const std::string& long_name) const; + // Registers the input name with the options and associated map. + // When classes register their options in this manner, most of the + // functionality (excluding unknown options and validate/prepare) is + // implemented by the base class. + // + // This method should be called in the class constructor to register the + // option set for this object. For example, to register the options + // associated with the BlockBasedTableFactory, the constructor calls this + // method passing in: + // - the name of the options ("BlockBasedTableOptions"); + // - the options object (the BlockBasedTableOptions object for this object; + // - the options type map for the BlockBasedTableOptions. + // This registration allows the Configurable class to process the option + // values associated with the BlockBasedTableOptions without further code in + // the derived class. + // + // @param name The name of this set of options (@see GetOptionsPtr) + // @param opt_ptr Pointer to the options to associate with this name + // @param opt_map Options map that controls how this option is configured. + template + void RegisterOptions( + T* opt_ptr, + const std::unordered_map* opt_map) { + RegisterOptions(T::kName(), opt_ptr, opt_map); + } + void RegisterOptions( + const std::string& name, void* opt_ptr, + const std::unordered_map* opt_map); + private: // Contains the collection of options (name, opt_ptr, opt_map) associated with // this object. This collection is typically set in the constructor of the diff --git a/options/cf_options.cc b/options/cf_options.cc index 64a5ae7fb..6332f3035 100644 --- a/options/cf_options.cc +++ b/options/cf_options.cc @@ -687,10 +687,9 @@ const std::string OptionsHelper::kCFOptionsName = "ColumnFamilyOptions"; class ConfigurableMutableCFOptions : public Configurable { public: - ConfigurableMutableCFOptions(const MutableCFOptions& mcf) { + explicit ConfigurableMutableCFOptions(const MutableCFOptions& mcf) { mutable_ = mcf; - ConfigurableHelper::RegisterOptions(*this, &mutable_, - &cf_mutable_options_type_info); + RegisterOptions(&mutable_, &cf_mutable_options_type_info); } protected: @@ -705,8 +704,7 @@ class ConfigurableCFOptions : public ConfigurableMutableCFOptions { immutable_(ImmutableDBOptions(), opts), cf_options_(opts), opt_map_(map) { - ConfigurableHelper::RegisterOptions(*this, &immutable_, - &cf_immutable_options_type_info); + RegisterOptions(&immutable_, &cf_immutable_options_type_info); } protected: diff --git a/options/configurable.cc b/options/configurable.cc index aa2b957c4..f425f193c 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -17,10 +17,10 @@ namespace ROCKSDB_NAMESPACE { -void ConfigurableHelper::RegisterOptions( - Configurable& configurable, const std::string& name, void* opt_ptr, +void Configurable::RegisterOptions( + const std::string& name, void* opt_ptr, const std::unordered_map* type_map) { - Configurable::RegisteredOptions opts; + RegisteredOptions opts; opts.name = name; #ifndef ROCKSDB_LITE opts.type_map = type_map; @@ -28,7 +28,7 @@ void ConfigurableHelper::RegisterOptions( (void)type_map; #endif // ROCKSDB_LITE opts.opt_ptr = opt_ptr; - configurable.options_.emplace_back(opts); + options_.emplace_back(opts); } //************************************************************************* diff --git a/options/configurable_helper.h b/options/configurable_helper.h index bc54a099f..b822b0b8e 100644 --- a/options/configurable_helper.h +++ b/options/configurable_helper.h @@ -22,35 +22,6 @@ class ConfigurableHelper { public: constexpr static const char* kIdPropName = "id"; constexpr static const char* kIdPropSuffix = ".id"; - // Registers the input name with the options and associated map. - // When classes register their options in this manner, most of the - // functionality (excluding unknown options and validate/prepare) is - // implemented by the base class. - // - // This method should be called in the class constructor to register the - // option set for this object. For example, to register the options - // associated with the BlockBasedTableFactory, the constructor calls this - // method passing in: - // - the name of the options ("BlockBasedTableOptions"); - // - the options object (the BlockBasedTableOptions object for this object; - // - the options type map for the BlockBasedTableOptions. - // This registration allows the Configurable class to process the option - // values associated with the BlockBasedTableOptions without further code in - // the derived class. - // - // @param name The name of this set of options (@see GetOptionsPtr) - // @param opt_ptr Pointer to the options to associate with this name - // @param opt_map Options map that controls how this option is configured. - template - static void RegisterOptions( - Configurable& configurable, T* opt_ptr, - const std::unordered_map* opt_map) { - RegisterOptions(configurable, T::kName(), opt_ptr, opt_map); - } - static void RegisterOptions( - Configurable& configurable, const std::string& name, void* opt_ptr, - const std::unordered_map* opt_map); - // Configures the input Configurable object based on the parameters. // On successful completion, the Configurable is updated with the settings // from the opt_map. diff --git a/options/configurable_test.cc b/options/configurable_test.cc index 68fcf963f..5983e2dc6 100644 --- a/options/configurable_test.cc +++ b/options/configurable_test.cc @@ -78,18 +78,15 @@ class SimpleConfigurable : public TestConfigurable { : TestConfigurable(name, mode, map) { if ((mode & TestConfigMode::kUniqueMode) != 0) { unique_.reset(SimpleConfigurable::Create("Unique" + name_)); - ConfigurableHelper::RegisterOptions(*this, name_ + "Unique", &unique_, - &unique_option_info); + RegisterOptions(name_ + "Unique", &unique_, &unique_option_info); } if ((mode & TestConfigMode::kSharedMode) != 0) { shared_.reset(SimpleConfigurable::Create("Shared" + name_)); - ConfigurableHelper::RegisterOptions(*this, name_ + "Shared", &shared_, - &shared_option_info); + RegisterOptions(name_ + "Shared", &shared_, &shared_option_info); } if ((mode & TestConfigMode::kRawPtrMode) != 0) { pointer_ = SimpleConfigurable::Create("Pointer" + name_); - ConfigurableHelper::RegisterOptions(*this, name_ + "Pointer", &pointer_, - &pointer_option_info); + RegisterOptions(name_ + "Pointer", &pointer_, &pointer_option_info); } } @@ -250,19 +247,15 @@ class ValidatedConfigurable : public SimpleConfigurable { : SimpleConfigurable(name, TestConfigMode::kDefaultMode), validated(false), prepared(0) { - ConfigurableHelper::RegisterOptions(*this, "Validated", &validated, - &validated_option_info); - ConfigurableHelper::RegisterOptions(*this, "Prepared", &prepared, - &prepared_option_info); + RegisterOptions("Validated", &validated, &validated_option_info); + RegisterOptions("Prepared", &prepared, &prepared_option_info); if ((mode & TestConfigMode::kUniqueMode) != 0) { unique_.reset(new ValidatedConfigurable( "Unique" + name_, TestConfigMode::kDefaultMode, false)); if (dont_prepare) { - ConfigurableHelper::RegisterOptions(*this, name_ + "Unique", &unique_, - &dont_prepare_option_info); + RegisterOptions(name_ + "Unique", &unique_, &dont_prepare_option_info); } else { - ConfigurableHelper::RegisterOptions(*this, name_ + "Unique", &unique_, - &unique_option_info); + RegisterOptions(name_ + "Unique", &unique_, &unique_option_info); } } } @@ -353,10 +346,8 @@ TEST_F(ConfigurableTest, MutableOptionsTest) { : SimpleConfigurable("mutable", TestConfigMode::kDefaultMode | TestConfigMode::kUniqueMode | TestConfigMode::kSharedMode) { - ConfigurableHelper::RegisterOptions(*this, "struct", &options_, - &struct_option_info); - ConfigurableHelper::RegisterOptions(*this, "imm", &options_, - &imm_option_info); + RegisterOptions("struct", &options_, &struct_option_info); + RegisterOptions("imm", &options_, &imm_option_info); } }; MutableConfigurable mc; diff --git a/options/configurable_test.h b/options/configurable_test.h index 52c3599f6..cf9d06678 100644 --- a/options/configurable_test.h +++ b/options/configurable_test.h @@ -112,11 +112,10 @@ class TestConfigurable : public Configurable { : name_(name), pointer_(nullptr) { prefix_ = "test." + name + "."; if ((mode & TestConfigMode::kSimpleMode) != 0) { - ConfigurableHelper::RegisterOptions(*this, name_, &options_, map); + RegisterOptions(name_, &options_, map); } if ((mode & TestConfigMode::kEnumMode) != 0) { - ConfigurableHelper::RegisterOptions(*this, name_ + "Enum", &options_, - &enum_option_info); + RegisterOptions(name_ + "Enum", &options_, &enum_option_info); } } diff --git a/options/customizable_helper.h b/options/customizable_helper.h index ff07e5498..2a2c12b62 100644 --- a/options/customizable_helper.h +++ b/options/customizable_helper.h @@ -9,7 +9,6 @@ #include #include "options/configurable_helper.h" -#include "options/options_helper.h" #include "rocksdb/convenience.h" #include "rocksdb/customizable.h" #include "rocksdb/status.h" diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 4726e4866..48e0ab244 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -98,8 +98,9 @@ static std::unordered_map a_option_info = { }; class ACustomizable : public TestCustomizable { public: - ACustomizable(const std::string& id) : TestCustomizable("A"), id_(id) { - ConfigurableHelper::RegisterOptions(*this, "A", &opts_, &a_option_info); + explicit ACustomizable(const std::string& id) + : TestCustomizable("A"), id_(id) { + RegisterOptions("A", &opts_, &a_option_info); } std::string GetId() const override { return id_; } static const char* kClassName() { return "A"; } @@ -141,8 +142,8 @@ static std::unordered_map b_option_info = { class BCustomizable : public TestCustomizable { private: public: - BCustomizable(const std::string& name) : TestCustomizable(name) { - ConfigurableHelper::RegisterOptions(*this, name, &opts_, &b_option_info); + explicit BCustomizable(const std::string& name) : TestCustomizable(name) { + RegisterOptions(name, &opts_, &b_option_info); } static const char* kClassName() { return "B"; } @@ -246,13 +247,12 @@ class SimpleConfigurable : public Configurable { public: SimpleConfigurable() { - ConfigurableHelper::RegisterOptions(*this, "simple", &simple_, - &simple_option_info); + RegisterOptions("simple", &simple_, &simple_option_info); } - SimpleConfigurable( + explicit SimpleConfigurable( const std::unordered_map* map) { - ConfigurableHelper::RegisterOptions(*this, "simple", &simple_, map); + RegisterOptions("simple", &simple_, map); } bool IsPrepared() const override { @@ -509,8 +509,7 @@ class ShallowCustomizable : public Customizable { public: ShallowCustomizable() { inner_ = std::make_shared("a"); - ConfigurableHelper::RegisterOptions(*this, "inner", &inner_, - &inner_option_info); + RegisterOptions("inner", &inner_, &inner_option_info); }; static const char* kClassName() { return "shallow"; } const char* Name() const override { return kClassName(); } @@ -641,10 +640,8 @@ TEST_F(CustomizableTest, MutableOptionsTest) { public: MutableCustomizable() { - ConfigurableHelper::RegisterOptions(*this, "mutable", &mutable_, - &mutable_option_info); - ConfigurableHelper::RegisterOptions(*this, "immutable", &immutable_, - &immutable_option_info); + RegisterOptions("mutable", &mutable_, &mutable_option_info); + RegisterOptions("immutable", &immutable_, &immutable_option_info); } const char* Name() const override { return "MutableCustomizable"; } }; diff --git a/options/db_options.cc b/options/db_options.cc index af88e41d4..ca1414758 100644 --- a/options/db_options.cc +++ b/options/db_options.cc @@ -435,10 +435,9 @@ const std::string OptionsHelper::kDBOptionsName = "DBOptions"; class MutableDBConfigurable : public Configurable { public: - MutableDBConfigurable(const MutableDBOptions& mdb) { + explicit MutableDBConfigurable(const MutableDBOptions& mdb) { mutable_ = mdb; - ConfigurableHelper::RegisterOptions(*this, &mutable_, - &db_mutable_options_type_info); + RegisterOptions(&mutable_, &db_mutable_options_type_info); } protected: @@ -447,7 +446,7 @@ class MutableDBConfigurable : public Configurable { class DBOptionsConfigurable : public MutableDBConfigurable { public: - DBOptionsConfigurable(const DBOptions& opts) + explicit DBOptionsConfigurable(const DBOptions& opts) : MutableDBConfigurable(MutableDBOptions(opts)), db_options_(opts) { // The ImmutableDBOptions currently requires the env to be non-null. Make // sure it is @@ -458,8 +457,7 @@ class DBOptionsConfigurable : public MutableDBConfigurable { copy.env = Env::Default(); immutable_ = ImmutableDBOptions(copy); } - ConfigurableHelper::RegisterOptions(*this, &immutable_, - &db_immutable_options_type_info); + RegisterOptions(&immutable_, &db_immutable_options_type_info); } protected: diff --git a/table/block_based/block_based_table_factory.cc b/table/block_based/block_based_table_factory.cc index d7ee41dc9..e9fb137f1 100644 --- a/table/block_based/block_based_table_factory.cc +++ b/table/block_based/block_based_table_factory.cc @@ -428,8 +428,7 @@ BlockBasedTableFactory::BlockBasedTableFactory( const BlockBasedTableOptions& _table_options) : table_options_(_table_options) { InitializeOptions(); - ConfigurableHelper::RegisterOptions(*this, &table_options_, - &block_based_table_type_info); + RegisterOptions(&table_options_, &block_based_table_type_info); } void BlockBasedTableFactory::InitializeOptions() { diff --git a/table/cuckoo/cuckoo_table_factory.cc b/table/cuckoo/cuckoo_table_factory.cc index c6d3c377c..5fe9ef749 100644 --- a/table/cuckoo/cuckoo_table_factory.cc +++ b/table/cuckoo/cuckoo_table_factory.cc @@ -95,8 +95,7 @@ static std::unordered_map cuckoo_table_type_info = CuckooTableFactory::CuckooTableFactory(const CuckooTableOptions& table_options) : table_options_(table_options) { - ConfigurableHelper::RegisterOptions(*this, &table_options_, - &cuckoo_table_type_info); + RegisterOptions(&table_options_, &cuckoo_table_type_info); } TableFactory* NewCuckooTableFactory(const CuckooTableOptions& table_options) { diff --git a/table/plain/plain_table_factory.cc b/table/plain/plain_table_factory.cc index e0d0e69f6..67b4bd3e5 100644 --- a/table/plain/plain_table_factory.cc +++ b/table/plain/plain_table_factory.cc @@ -52,8 +52,7 @@ static std::unordered_map plain_table_type_info = { PlainTableFactory::PlainTableFactory(const PlainTableOptions& options) : table_options_(options) { - ConfigurableHelper::RegisterOptions(*this, &table_options_, - &plain_table_type_info); + RegisterOptions(&table_options_, &plain_table_type_info); } Status PlainTableFactory::NewTableReader(