From 9f2d255aedcc934b6ede0c9581ab6c6b61aa0326 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 11 May 2021 06:45:49 -0700 Subject: [PATCH] Add ObjectRegistry to ConfigOptions (#8166) Summary: This change enables a couple of things: - Different ConfigOptions can have different registry/factory associated with it, thereby allowing things like a "Test" ConfigOptions versus a "Production" - The ObjectRegistry is created fewer times and can be re-used The ConfigOptions can also be initialized/constructed from a DBOptions, in which case it will grab some of its settings (Env, Logger) from the DBOptions. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8166 Reviewed By: zhichao-cao Differential Revision: D27657952 Pulled By: mrambacher fbshipit-source-id: ae1d6200bb7ab127405cdeefaba43c7fe694dfdd --- HISTORY.md | 2 + db/db_options_test.cc | 2 +- include/rocksdb/convenience.h | 18 +++++- include/rocksdb/utilities/object_registry.h | 55 +++++++++++++++- options/cf_options.cc | 11 ++-- options/customizable_helper.h | 15 +++-- options/customizable_test.cc | 62 ++++++++++++++++++ options/options_helper.cc | 23 +++++-- options/options_test.cc | 69 ++++++++++++++++++--- utilities/object_registry.cc | 34 ++++++++-- utilities/object_registry_test.cc | 57 +++++++++++++++-- utilities/options/options_util.cc | 2 +- 12 files changed, 309 insertions(+), 41 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 163c6788f..91ced2d71 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -23,6 +23,8 @@ * Removed unused structure `CompactionFilterContext`. * The `skip_filters` parameter to SstFileWriter is now considered deprecated. Use `BlockBasedTableOptions::filter_policy` to control generation of filters. * ClockCache is known to have bugs that could lead to crash or corruption, so should not be used until fixed. Use NewLRUCache instead. +* Added the ObjectRegistry to the ConfigOptions class. This registry instance will be used to find any customizable loadable objects during initialization. +* Expanded the ObjectRegistry functionality to allow nested ObjectRegistry instances. Added methods to register a set of functions with the registry/library as a group. * Deprecated backupable_db.h and BackupableDBOptions in favor of new versions with appropriate names: backup_engine.h and BackupEngineOptions. Old API compatibility is preserved. ### Default Option Change diff --git a/db/db_options_test.cc b/db/db_options_test.cc index 0858e1421..96fd37357 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -34,7 +34,7 @@ class DBOptionsTest : public DBTestBase { const DBOptions& options) { std::string options_str; std::unordered_map mutable_map; - ConfigOptions config_options; + ConfigOptions config_options(options); config_options.delimiter = "; "; EXPECT_OK(GetStringFromMutableDBOptions( diff --git a/include/rocksdb/convenience.h b/include/rocksdb/convenience.h index 42ae08e2a..dfcd7e229 100644 --- a/include/rocksdb/convenience.h +++ b/include/rocksdb/convenience.h @@ -16,6 +16,9 @@ namespace ROCKSDB_NAMESPACE { class Env; +class Logger; +class ObjectRegistry; + struct ColumnFamilyOptions; struct DBOptions; struct Options; @@ -27,6 +30,15 @@ struct Options; // of the serialization (e.g. delimiter), and how to compare // options (sanity_level). struct ConfigOptions { + // Constructs a new ConfigOptions with a new object registry. + // This method should only be used when a DBOptions is not available, + // else registry settings may be lost + ConfigOptions(); + + // Constructs a new ConfigOptions using the settings from + // the input DBOptions. Currently constructs a new object registry. + explicit ConfigOptions(const DBOptions&); + // This enum defines the RocksDB options sanity level. enum SanityLevel : unsigned char { kSanityLevelNone = 0x01, // Performs no sanity check at all. @@ -78,6 +90,11 @@ struct ConfigOptions { // The environment to use for this option Env* env = Env::Default(); +#ifndef ROCKSDB_LITE + // The object registry to use for this options + std::shared_ptr registry; +#endif + bool IsShallow() const { return depth == Depth::kDepthShallow; } bool IsDetailed() const { return depth == Depth::kDepthDetailed; } @@ -500,7 +517,6 @@ Status VerifySstFileChecksum(const Options& options, const EnvOptions& env_options, const ReadOptions& read_options, const std::string& file_path); - #endif // ROCKSDB_LITE } // namespace ROCKSDB_NAMESPACE diff --git a/include/rocksdb/utilities/object_registry.h b/include/rocksdb/utilities/object_registry.h index 538cb6a8f..5a454d775 100644 --- a/include/rocksdb/utilities/object_registry.h +++ b/include/rocksdb/utilities/object_registry.h @@ -17,12 +17,23 @@ namespace ROCKSDB_NAMESPACE { class Logger; +class ObjectLibrary; + // Returns a new T when called with a string. Populates the std::unique_ptr // argument if granting ownership to caller. template using FactoryFunc = std::function*, std::string*)>; +// The signature of the function for loading factories +// into an object library. This method is expected to register +// factory functions in the supplied ObjectLibrary. +// The ObjectLibrary is the library in which the factories will be loaded. +// The std::string is the argument passed to the loader function. +// The RegistrarFunc should return the number of objects loaded into this +// library +using RegistrarFunc = std::function; + class ObjectLibrary { public: // Base class for an Entry in the Registry. @@ -62,9 +73,18 @@ class ObjectLibrary { FactoryFunc factory_; }; // End class FactoryEntry public: + explicit ObjectLibrary(const std::string& id) { id_ = id; } + + const std::string& GetID() const { return id_; } // Finds the entry matching the input name and type const Entry* FindEntry(const std::string& type, const std::string& name) const; + + // Returns the total number of factories registered for this library. + // This method returns the sum of all factories registered for all types. + // @param num_types returns how many unique types are registered. + size_t GetFactoryCount(size_t* num_types) const; + void Dump(Logger* logger) const; // Registers the factory with the library for the pattern. @@ -76,6 +96,12 @@ class ObjectLibrary { AddEntry(T::Type(), entry); return factory; } + + // Invokes the registrar function with the supplied arg for this library. + int Register(const RegistrarFunc& registrar, const std::string& arg) { + return registrar(*this, arg); + } + // Returns the default ObjectLibrary static std::shared_ptr& Default(); @@ -85,6 +111,9 @@ class ObjectLibrary { // ** FactoryFunctions for this loader, organized by type std::unordered_map>> entries_; + + // The name for this library + std::string id_; }; // The ObjectRegistry is used to register objects that can be created by a @@ -93,11 +122,26 @@ class ObjectLibrary { class ObjectRegistry { public: static std::shared_ptr NewInstance(); - - ObjectRegistry(); + static std::shared_ptr NewInstance( + const std::shared_ptr& parent); + static std::shared_ptr Default(); + explicit ObjectRegistry(const std::shared_ptr& parent) + : parent_(parent) {} + + std::shared_ptr AddLibrary(const std::string& id) { + auto library = std::make_shared(id); + libraries_.push_back(library); + return library; + } void AddLibrary(const std::shared_ptr& library) { - libraries_.emplace_back(library); + libraries_.push_back(library); + } + + void AddLibrary(const std::string& id, const RegistrarFunc& registrar, + const std::string& arg) { + auto library = AddLibrary(id); + library->Register(registrar, arg); } // Creates a new T using the factory function that was registered with a @@ -193,6 +237,10 @@ class ObjectRegistry { void Dump(Logger* logger) const; private: + explicit ObjectRegistry(const std::shared_ptr& library) { + libraries_.push_back(library); + } + const ObjectLibrary::Entry* FindEntry(const std::string& type, const std::string& name) const; @@ -200,6 +248,7 @@ class ObjectRegistry { // The libraries are searched in reverse order (back to front) when // searching for entries. std::vector> libraries_; + std::shared_ptr parent_; }; } // namespace ROCKSDB_NAMESPACE #endif // ROCKSDB_LITE diff --git a/options/cf_options.cc b/options/cf_options.cc index 9a62cc9df..a77a18016 100644 --- a/options/cf_options.cc +++ b/options/cf_options.cc @@ -541,12 +541,12 @@ static std::unordered_map OptionType::kComparator, OptionVerificationType::kByName, OptionTypeFlags::kCompareLoose, // Parses the string and sets the corresponding comparator - [](const ConfigOptions& /*opts*/, const std::string& /*name*/, + [](const ConfigOptions& opts, const std::string& /*name*/, const std::string& value, char* addr) { auto old_comparator = reinterpret_cast(addr); const Comparator* new_comparator = *old_comparator; - Status status = ObjectRegistry::NewInstance()->NewStaticObject( - value, &new_comparator); + Status status = + opts.registry->NewStaticObject(value, &new_comparator); if (status.ok()) { *old_comparator = new_comparator; return status; @@ -661,12 +661,11 @@ static std::unordered_map OptionVerificationType::kByNameAllowFromNull, OptionTypeFlags::kCompareLoose, // Parses the input value as a MergeOperator, updating the value - [](const ConfigOptions& /*opts*/, const std::string& /*name*/, + [](const ConfigOptions& opts, const std::string& /*name*/, const std::string& value, char* addr) { auto mop = reinterpret_cast*>(addr); Status status = - ObjectRegistry::NewInstance()->NewSharedObject( - value, mop); + opts.registry->NewSharedObject(value, mop); // Only support static comparator for now. if (status.ok()) { return status; diff --git a/options/customizable_helper.h b/options/customizable_helper.h index 2a2c12b62..cd7cc26f8 100644 --- a/options/customizable_helper.h +++ b/options/customizable_helper.h @@ -83,12 +83,13 @@ static Status LoadSharedObject(const ConfigOptions& config_options, return Status::NotSupported("Cannot reset object ", id); } else { #ifndef ROCKSDB_LITE - status = ObjectRegistry::NewInstance()->NewSharedObject(id, result); + status = config_options.registry->NewSharedObject(id, result); #else status = Status::NotSupported("Cannot load object in LITE mode ", id); #endif if (!status.ok()) { - if (config_options.ignore_unsupported_options) { + if (config_options.ignore_unsupported_options && + status.IsNotSupported()) { return Status::OK(); } else { return status; @@ -142,12 +143,13 @@ static Status LoadUniqueObject(const ConfigOptions& config_options, return Status::NotSupported("Cannot reset object ", id); } else { #ifndef ROCKSDB_LITE - status = ObjectRegistry::NewInstance()->NewUniqueObject(id, result); + status = config_options.registry->NewUniqueObject(id, result); #else status = Status::NotSupported("Cannot load object in LITE mode ", id); #endif // ROCKSDB_LITE if (!status.ok()) { - if (config_options.ignore_unsupported_options) { + if (config_options.ignore_unsupported_options && + status.IsNotSupported()) { return Status::OK(); } else { return status; @@ -199,12 +201,13 @@ static Status LoadStaticObject(const ConfigOptions& config_options, return Status::NotSupported("Cannot reset object ", id); } else { #ifndef ROCKSDB_LITE - status = ObjectRegistry::NewInstance()->NewStaticObject(id, result); + status = config_options.registry->NewStaticObject(id, result); #else status = Status::NotSupported("Cannot load object in LITE mode ", id); #endif // ROCKSDB_LITE if (!status.ok()) { - if (config_options.ignore_unsupported_options) { + if (config_options.ignore_unsupported_options && + status.IsNotSupported()) { return Status::OK(); } else { return status; diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 48e0ab244..9cf6b9126 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -695,6 +695,68 @@ TEST_F(CustomizableTest, MutableOptionsTest) { } #endif // !ROCKSDB_LITE +#ifndef ROCKSDB_LITE +// This method loads existing test classes into the ObjectRegistry +static int RegisterTestObjects(ObjectLibrary& library, + const std::string& /*arg*/) { + size_t num_types; + library.Register( + "MockTable", + [](const std::string& /*uri*/, std::unique_ptr* guard, + std::string* /* errmsg */) { + guard->reset(new mock::MockTableFactory()); + return guard->get(); + }); + return static_cast(library.GetFactoryCount(&num_types)); +} + +static int RegisterLocalObjects(ObjectLibrary& library, + const std::string& /*arg*/) { + size_t num_types; + // Load any locally defined objects here + return static_cast(library.GetFactoryCount(&num_types)); +} + +class LoadCustomizableTest : public testing::Test { + public: + LoadCustomizableTest() { config_options_.ignore_unsupported_options = false; } + bool RegisterTests(const std::string& arg) { +#ifndef ROCKSDB_LITE + config_options_.registry->AddLibrary("custom-tests", RegisterTestObjects, + arg); + config_options_.registry->AddLibrary("local-tests", RegisterLocalObjects, + arg); + return true; +#else + (void)arg; + return false; +#endif // !ROCKSDB_LITE + } + + protected: + DBOptions db_opts_; + ColumnFamilyOptions cf_opts_; + ConfigOptions config_options_; +}; + +TEST_F(LoadCustomizableTest, LoadTableFactoryTest) { + std::shared_ptr factory; + ASSERT_NOK( + TableFactory::CreateFromString(config_options_, "MockTable", &factory)); + ASSERT_OK(TableFactory::CreateFromString( + config_options_, TableFactory::kBlockBasedTableName(), &factory)); + ASSERT_NE(factory, nullptr); + ASSERT_STREQ(factory->Name(), TableFactory::kBlockBasedTableName()); + + if (RegisterTests("Test")) { + ASSERT_OK( + TableFactory::CreateFromString(config_options_, "MockTable", &factory)); + ASSERT_NE(factory, nullptr); + ASSERT_STREQ(factory->Name(), "MockTable"); + } +} +#endif // !ROCKSDB_LITE + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); diff --git a/options/options_helper.cc b/options/options_helper.cc index f56ac1b2e..5ee8b4a7f 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -28,6 +28,20 @@ #include "util/string_util.h" namespace ROCKSDB_NAMESPACE { +ConfigOptions::ConfigOptions() +#ifndef ROCKSDB_LITE + : registry(ObjectRegistry::NewInstance()) +#endif +{ + env = Env::Default(); +} + +ConfigOptions::ConfigOptions(const DBOptions& db_opts) : env(db_opts.env) { +#ifndef ROCKSDB_LITE + registry = ObjectRegistry::NewInstance(); +#endif +} + Status ValidateOptions(const DBOptions& db_opts, const ColumnFamilyOptions& cf_opts) { Status s; @@ -703,7 +717,7 @@ Status GetStringFromMutableDBOptions(const ConfigOptions& config_options, Status GetStringFromDBOptions(std::string* opt_string, const DBOptions& db_options, const std::string& delimiter) { - ConfigOptions config_options; + ConfigOptions config_options(db_options); config_options.delimiter = delimiter; return GetStringFromDBOptions(config_options, db_options, opt_string); } @@ -816,7 +830,7 @@ Status GetDBOptionsFromMap( const std::unordered_map& opts_map, DBOptions* new_options, bool input_strings_escaped, bool ignore_unknown_options) { - ConfigOptions config_options; + ConfigOptions config_options(base_options); config_options.input_strings_escaped = input_strings_escaped; config_options.ignore_unknown_options = ignore_unknown_options; return GetDBOptionsFromMap(config_options, base_options, opts_map, @@ -844,7 +858,7 @@ Status GetDBOptionsFromMap( Status GetDBOptionsFromString(const DBOptions& base_options, const std::string& opts_str, DBOptions* new_options) { - ConfigOptions config_options; + ConfigOptions config_options(base_options); config_options.input_strings_escaped = false; config_options.ignore_unknown_options = false; @@ -868,7 +882,7 @@ Status GetDBOptionsFromString(const ConfigOptions& config_options, Status GetOptionsFromString(const Options& base_options, const std::string& opts_str, Options* new_options) { - ConfigOptions config_options; + ConfigOptions config_options(base_options); config_options.input_strings_escaped = false; config_options.ignore_unknown_options = false; @@ -883,6 +897,7 @@ Status GetOptionsFromString(const ConfigOptions& config_options, std::unordered_map unused_opts; std::unordered_map opts_map; + assert(new_options); *new_options = base_options; Status s = StringToMap(opts_str, &opts_map); if (!s.ok()) { diff --git a/options/options_test.cc b/options/options_test.cc index e3486ac41..e87d887ac 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -1808,6 +1808,23 @@ TEST_F(OptionsTest, ConvertOptionsTest) { } #ifndef ROCKSDB_LITE +const static std::string kCustomEnvName = "Custom"; +const static std::string kCustomEnvProp = "env=" + kCustomEnvName; +class CustomEnv : public EnvWrapper { + public: + explicit CustomEnv(Env* _target) : EnvWrapper(_target) {} +}; + +static int RegisterCustomEnv(ObjectLibrary& library, const std::string& arg) { + library.Register( + arg, [](const std::string& /*name*/, std::unique_ptr* /*env_guard*/, + std::string* /* errmsg */) { + static CustomEnv env(Env::Default()); + return &env; + }); + return 1; +} + // This test suite tests the old APIs into the Configure options methods. // Once those APIs are officially deprecated, this test suite can be deleted. class OptionsOldApiTest : public testing::Test {}; @@ -2507,14 +2524,8 @@ TEST_F(OptionsOldApiTest, GetOptionsFromStringTest) { NewBlockBasedTableFactory(block_based_table_options)); // Register an Env with object registry. - const static char* kCustomEnvName = "CustomEnv"; - class CustomEnv : public EnvWrapper { - public: - explicit CustomEnv(Env* _target) : EnvWrapper(_target) {} - }; - ObjectLibrary::Default()->Register( - kCustomEnvName, + "CustomEnvDefault", [](const std::string& /*name*/, std::unique_ptr* /*env_guard*/, std::string* /* errmsg */) { static CustomEnv env(Env::Default()); @@ -2528,7 +2539,7 @@ TEST_F(OptionsOldApiTest, GetOptionsFromStringTest) { "compression_opts=4:5:6;create_if_missing=true;max_open_files=1;" "bottommost_compression_opts=5:6:7;create_if_missing=true;max_open_files=" "1;" - "rate_limiter_bytes_per_sec=1024;env=CustomEnv", + "rate_limiter_bytes_per_sec=1024;env=CustomEnvDefault", &new_options)); ASSERT_EQ(new_options.compression_opts.window_bits, 4); @@ -2562,7 +2573,7 @@ TEST_F(OptionsOldApiTest, GetOptionsFromStringTest) { ASSERT_EQ(new_options.max_open_files, 1); ASSERT_TRUE(new_options.rate_limiter.get() != nullptr); Env* newEnv = new_options.env; - ASSERT_OK(Env::LoadEnv(kCustomEnvName, &newEnv)); + ASSERT_OK(Env::LoadEnv("CustomEnvDefault", &newEnv)); ASSERT_EQ(newEnv, new_options.env); } @@ -3981,6 +3992,46 @@ TEST_F(OptionTypeInfoTest, TestVectorType) { ASSERT_EQ(vec1[1], "b1|b2"); ASSERT_EQ(vec1[2], "c1|c2|{d1|d2}"); } + +class ConfigOptionsTest : public testing::Test {}; + +TEST_F(ConfigOptionsTest, EnvFromConfigOptions) { + ConfigOptions config_options; + DBOptions db_opts; + Options opts; + Env* mem_env = NewMemEnv(Env::Default()); + config_options.registry->AddLibrary("custom-env", RegisterCustomEnv, + kCustomEnvName); + + config_options.env = mem_env; + // First test that we can get the env as expected + ASSERT_OK(GetDBOptionsFromString(config_options, DBOptions(), kCustomEnvProp, + &db_opts)); + ASSERT_OK( + GetOptionsFromString(config_options, Options(), kCustomEnvProp, &opts)); + ASSERT_NE(config_options.env, db_opts.env); + ASSERT_EQ(opts.env, db_opts.env); + Env* custom_env = db_opts.env; + + // Now try a "bad" env" and check that nothing changed + config_options.ignore_unsupported_options = true; + ASSERT_OK( + GetDBOptionsFromString(config_options, db_opts, "env=unknown", &db_opts)); + ASSERT_OK(GetOptionsFromString(config_options, opts, "env=unknown", &opts)); + ASSERT_EQ(config_options.env, mem_env); + ASSERT_EQ(db_opts.env, custom_env); + ASSERT_EQ(opts.env, db_opts.env); + + // Now try a "bad" env" ignoring unknown objects + config_options.ignore_unsupported_options = false; + ASSERT_NOK( + GetDBOptionsFromString(config_options, db_opts, "env=unknown", &db_opts)); + ASSERT_EQ(config_options.env, mem_env); + ASSERT_EQ(db_opts.env, custom_env); + ASSERT_EQ(opts.env, db_opts.env); + + delete mem_env; +} #endif // !ROCKSDB_LITE } // namespace ROCKSDB_NAMESPACE diff --git a/utilities/object_registry.cc b/utilities/object_registry.cc index 38e55020e..d74d37d17 100644 --- a/utilities/object_registry.cc +++ b/utilities/object_registry.cc @@ -32,6 +32,15 @@ void ObjectLibrary::AddEntry(const std::string &type, entries.emplace_back(std::move(entry)); } +size_t ObjectLibrary::GetFactoryCount(size_t *types) const { + *types = entries_.size(); + size_t factories = 0; + for (const auto &e : entries_) { + factories += e.second.size(); + } + return factories; +} + void ObjectLibrary::Dump(Logger *logger) const { for (const auto &iter : entries_) { ROCKS_LOG_HEADER(logger, " Registered factories for type[%s] ", @@ -50,17 +59,23 @@ void ObjectLibrary::Dump(Logger *logger) const { // This instance will contain most of the "standard" registered objects std::shared_ptr &ObjectLibrary::Default() { static std::shared_ptr instance = - std::make_shared(); + std::make_shared("default"); return instance; } -std::shared_ptr ObjectRegistry::NewInstance() { - std::shared_ptr instance = std::make_shared(); +std::shared_ptr ObjectRegistry::Default() { + static std::shared_ptr instance( + new ObjectRegistry(ObjectLibrary::Default())); return instance; } -ObjectRegistry::ObjectRegistry() { - libraries_.push_back(ObjectLibrary::Default()); +std::shared_ptr ObjectRegistry::NewInstance() { + return std::make_shared(Default()); +} + +std::shared_ptr ObjectRegistry::NewInstance( + const std::shared_ptr &parent) { + return std::make_shared(parent); } // Searches (from back to front) the libraries looking for the @@ -74,13 +89,20 @@ const ObjectLibrary::Entry *ObjectRegistry::FindEntry( return entry; } } - return nullptr; + if (parent_ != nullptr) { + return parent_->FindEntry(type, name); + } else { + return nullptr; + } } void ObjectRegistry::Dump(Logger *logger) const { for (auto iter = libraries_.crbegin(); iter != libraries_.crend(); ++iter) { iter->get()->Dump(logger); } + if (parent_ != nullptr) { + parent_->Dump(logger); + } } #endif // ROCKSDB_LITE diff --git a/utilities/object_registry_test.cc b/utilities/object_registry_test.cc index bbb76b142..e09392dd8 100644 --- a/utilities/object_registry_test.cc +++ b/utilities/object_registry_test.cc @@ -62,7 +62,8 @@ TEST_F(EnvRegistryTest, LocalRegistry) { std::string msg; std::unique_ptr guard; auto registry = ObjectRegistry::NewInstance(); - std::shared_ptr library = std::make_shared(); + std::shared_ptr library = + std::make_shared("local"); registry->AddLibrary(library); library->Register( "test-local", @@ -87,7 +88,8 @@ TEST_F(EnvRegistryTest, LocalRegistry) { TEST_F(EnvRegistryTest, CheckShared) { std::shared_ptr shared; std::shared_ptr registry = ObjectRegistry::NewInstance(); - std::shared_ptr library = std::make_shared(); + std::shared_ptr library = + std::make_shared("shared"); registry->AddLibrary(library); library->Register( "unguarded", @@ -111,7 +113,8 @@ TEST_F(EnvRegistryTest, CheckShared) { TEST_F(EnvRegistryTest, CheckStatic) { Env* env = nullptr; std::shared_ptr registry = ObjectRegistry::NewInstance(); - std::shared_ptr library = std::make_shared(); + std::shared_ptr library = + std::make_shared("static"); registry->AddLibrary(library); library->Register( "unguarded", @@ -135,7 +138,8 @@ TEST_F(EnvRegistryTest, CheckStatic) { TEST_F(EnvRegistryTest, CheckUnique) { std::unique_ptr unique; std::shared_ptr registry = ObjectRegistry::NewInstance(); - std::shared_ptr library = std::make_shared(); + std::shared_ptr library = + std::make_shared("unique"); registry->AddLibrary(library); library->Register( "unguarded", @@ -156,6 +160,51 @@ TEST_F(EnvRegistryTest, CheckUnique) { ASSERT_EQ(unique, nullptr); } +TEST_F(EnvRegistryTest, TestRegistryParents) { + auto grand = ObjectRegistry::Default(); + auto parent = ObjectRegistry::NewInstance(); // parent with a grandparent + auto uncle = ObjectRegistry::NewInstance(grand); + auto child = ObjectRegistry::NewInstance(parent); + auto cousin = ObjectRegistry::NewInstance(uncle); + + auto library = parent->AddLibrary("parent"); + library->Register( + "parent", [](const std::string& /*uri*/, std::unique_ptr* guard, + std::string* /* errmsg */) { + guard->reset(new EnvWrapper(Env::Default())); + return guard->get(); + }); + library = cousin->AddLibrary("cousin"); + library->Register( + "cousin", [](const std::string& /*uri*/, std::unique_ptr* guard, + std::string* /* errmsg */) { + guard->reset(new EnvWrapper(Env::Default())); + return guard->get(); + }); + + std::unique_ptr guard; + std::string msg; + + // a:://* is registered in Default, so they should all workd + ASSERT_NE(parent->NewObject("a://test", &guard, &msg), nullptr); + ASSERT_NE(child->NewObject("a://test", &guard, &msg), nullptr); + ASSERT_NE(uncle->NewObject("a://test", &guard, &msg), nullptr); + ASSERT_NE(cousin->NewObject("a://test", &guard, &msg), nullptr); + + // The parent env is only registered for parent, not uncle, + // So parent and child should return success and uncle and cousin should fail + ASSERT_OK(parent->NewUniqueObject("parent", &guard)); + ASSERT_OK(child->NewUniqueObject("parent", &guard)); + ASSERT_NOK(uncle->NewUniqueObject("parent", &guard)); + ASSERT_NOK(cousin->NewUniqueObject("parent", &guard)); + + // The cousin is only registered in the cousin, so all of the others should + // fail + ASSERT_OK(cousin->NewUniqueObject("cousin", &guard)); + ASSERT_NOK(parent->NewUniqueObject("cousin", &guard)); + ASSERT_NOK(child->NewUniqueObject("cousin", &guard)); + ASSERT_NOK(uncle->NewUniqueObject("cousin", &guard)); +} } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { diff --git a/utilities/options/options_util.cc b/utilities/options/options_util.cc index 2a6a83aee..9efbf76f0 100644 --- a/utilities/options/options_util.cc +++ b/utilities/options/options_util.cc @@ -121,7 +121,7 @@ Status CheckOptionsCompatibility( const std::string& dbpath, Env* env, const DBOptions& db_options, const std::vector& cf_descs, bool ignore_unknown_options) { - ConfigOptions config_options; + ConfigOptions config_options(db_options); config_options.sanity_level = ConfigOptions::kSanityLevelLooselyCompatible; config_options.ignore_unknown_options = ignore_unknown_options; config_options.input_strings_escaped = true;