diff --git a/HISTORY.md b/HISTORY.md index fb27f9591..5e510ee2e 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -58,6 +58,7 @@ * Remove default implementation of Name() from FileSystemWrapper. * Rename `SizeApproximationOptions.include_memtabtles` to `SizeApproximationOptions.include_memtables`. * Remove deprecated option DBOptions::max_mem_compaction_level. +* Return Status::InvalidArgument from ObjectRegistry::NewObject if a factory exists but the object ould not be created (returns NotFound if the factory is missing). * Remove deprecated overloads of API DB::GetApproximateSizes. * Remove deprecated option DBOptions::new_table_reader_for_compaction_inputs. diff --git a/env/env.cc b/env/env.cc index 186429c15..dd214f655 100644 --- a/env/env.cc +++ b/env/env.cc @@ -687,12 +687,8 @@ Status Env::CreateFromString(const ConfigOptions& config_options, } else { RegisterSystemEnvs(); #ifndef ROCKSDB_LITE - std::string errmsg; - env = config_options.registry->NewObject(id, &uniq, &errmsg); - if (!env) { - status = Status::NotSupported( - std::string("Cannot load environment[") + id + "]: ", errmsg); - } + // First, try to load the Env as a unique object. + status = config_options.registry->NewObject(id, &env, &uniq); #else status = Status::NotSupported("Cannot load environment in LITE mode", value); diff --git a/include/rocksdb/utilities/object_registry.h b/include/rocksdb/utilities/object_registry.h index 928676b2e..9730ed1b1 100644 --- a/include/rocksdb/utilities/object_registry.h +++ b/include/rocksdb/utilities/object_registry.h @@ -296,29 +296,31 @@ class ObjectRegistry { library->Register(registrar, arg); } - // Creates a new T using the factory function that was registered for this - // target. Searches through the libraries to find the first library where - // there is an entry that matches target (see PatternEntry for the matching - // rules). - // - // If no registered functions match, returns nullptr. If multiple functions - // match, the factory function used is unspecified. - // - // Populates guard with result pointer if caller is granted ownership. - // Deprecated. Use NewShared/Static/UniqueObject instead. + // Finds the factory for target and instantiates a new T. + // Returns NotSupported if no factory is found + // Returns InvalidArgument if a factory is found but the factory failed. template - T* NewObject(const std::string& target, std::unique_ptr* guard, - std::string* errmsg) { + Status NewObject(const std::string& target, T** object, + std::unique_ptr* guard) { + assert(guard != nullptr); guard->reset(); auto factory = FindFactory(target); if (factory != nullptr) { - return factory(target, guard, errmsg); + std::string errmsg; + *object = factory(target, guard, &errmsg); + if (*object != nullptr) { + return Status::OK(); + } else if (errmsg.empty()) { + return Status::InvalidArgument( + std::string("Could not load ") + T::Type(), target); + } else { + return Status::InvalidArgument(errmsg, target); + } } else { - *errmsg = std::string("Could not load ") + T::Type(); - return nullptr; + return Status::NotSupported(std::string("Could not load ") + T::Type(), + target); } } - // Creates a new unique T using the input factory functions. // Returns OK if a new unique T was successfully created // Returns NotSupported if the type/target could not be created @@ -327,11 +329,13 @@ class ObjectRegistry { template Status NewUniqueObject(const std::string& target, std::unique_ptr* result) { - std::string errmsg; - T* ptr = NewObject(target, result, &errmsg); - if (ptr == nullptr) { - return Status::NotSupported(errmsg, target); - } else if (*result) { + T* ptr = nullptr; + std::unique_ptr guard; + Status s = NewObject(target, &ptr, &guard); + if (!s.ok()) { + return s; + } else if (guard) { + result->reset(guard.release()); return Status::OK(); } else { return Status::InvalidArgument(std::string("Cannot make a unique ") + @@ -348,11 +352,11 @@ class ObjectRegistry { template Status NewSharedObject(const std::string& target, std::shared_ptr* result) { - std::string errmsg; std::unique_ptr guard; - T* ptr = NewObject(target, &guard, &errmsg); - if (ptr == nullptr) { - return Status::NotSupported(errmsg, target); + T* ptr = nullptr; + Status s = NewObject(target, &ptr, &guard); + if (!s.ok()) { + return s; } else if (guard) { result->reset(guard.release()); return Status::OK(); @@ -370,11 +374,11 @@ class ObjectRegistry { // (meaning it is managed by a unique ptr) template Status NewStaticObject(const std::string& target, T** result) { - std::string errmsg; std::unique_ptr guard; - T* ptr = NewObject(target, &guard, &errmsg); - if (ptr == nullptr) { - return Status::NotSupported(errmsg, target); + T* ptr = nullptr; + Status s = NewObject(target, &ptr, &guard); + if (!s.ok()) { + return s; } else if (guard.get()) { return Status::InvalidArgument(std::string("Cannot make a static ") + T::Type() + " from a guarded one ", diff --git a/memory/memory_allocator_test.cc b/memory/memory_allocator_test.cc index 90aed448b..1e96c44ee 100644 --- a/memory/memory_allocator_test.cc +++ b/memory/memory_allocator_test.cc @@ -30,11 +30,7 @@ class MemoryAllocatorTest std::tie(id_, supported_) = GetParam(); Status s = MemoryAllocator::CreateFromString(ConfigOptions(), id_, &allocator_); - if (supported_) { - EXPECT_OK(s); - } else if (!s.ok()) { - EXPECT_TRUE(s.IsNotSupported()); - } + EXPECT_EQ(supported_, s.ok()); } bool IsSupported() { return supported_; } @@ -140,7 +136,7 @@ TEST_F(CreateMemoryAllocatorTest, JemallocOptionsTest) { std::string id = std::string("id=") + JemallocNodumpAllocator::kClassName(); Status s = MemoryAllocator::CreateFromString(config_options_, id, &allocator); if (!JemallocNodumpAllocator::IsSupported()) { - ASSERT_TRUE(s.IsNotSupported()); + ASSERT_NOK(s); ROCKSDB_GTEST_BYPASS("JEMALLOC not supported"); return; } @@ -192,7 +188,7 @@ TEST_F(CreateMemoryAllocatorTest, NewJemallocNodumpAllocator) { Status s = NewJemallocNodumpAllocator(jopts, &allocator); std::string msg; if (!JemallocNodumpAllocator::IsSupported(&msg)) { - ASSERT_TRUE(s.IsNotSupported()); + ASSERT_NOK(s); ROCKSDB_GTEST_BYPASS("JEMALLOC not supported"); return; } diff --git a/options/customizable_test.cc b/options/customizable_test.cc index c61a51243..92fe527f2 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -497,6 +497,55 @@ TEST_F(CustomizableTest, BadOptionTest) { ASSERT_OK(c1->ConfigureFromString(ignore, "shared.id=A;A.string=s}")); } +TEST_F(CustomizableTest, FailingFactoryTest) { + std::shared_ptr registry = ObjectRegistry::NewInstance(); + std::unique_ptr c1(new SimpleConfigurable()); + ConfigOptions ignore = config_options_; + + Status s; + ignore.registry->AddLibrary("failing")->AddFactory( + "failing", + [](const std::string& /*uri*/, + std::unique_ptr* /*guard */, std::string* errmsg) { + *errmsg = "Bad Factory"; + return nullptr; + }); + + // If we are ignoring unknown and unsupported options, will see + // different errors for failing versus missing + ignore.ignore_unknown_options = false; + ignore.ignore_unsupported_options = false; + s = c1->ConfigureFromString(ignore, "shared.id=failing"); + ASSERT_TRUE(s.IsInvalidArgument()); + s = c1->ConfigureFromString(ignore, "unique.id=failing"); + ASSERT_TRUE(s.IsInvalidArgument()); + s = c1->ConfigureFromString(ignore, "shared.id=missing"); + ASSERT_TRUE(s.IsNotSupported()); + s = c1->ConfigureFromString(ignore, "unique.id=missing"); + ASSERT_TRUE(s.IsNotSupported()); + + // If we are ignoring unsupported options, will see + // errors for failing but not missing + ignore.ignore_unknown_options = false; + ignore.ignore_unsupported_options = true; + s = c1->ConfigureFromString(ignore, "shared.id=failing"); + ASSERT_TRUE(s.IsInvalidArgument()); + s = c1->ConfigureFromString(ignore, "unique.id=failing"); + ASSERT_TRUE(s.IsInvalidArgument()); + + ASSERT_OK(c1->ConfigureFromString(ignore, "shared.id=missing")); + ASSERT_OK(c1->ConfigureFromString(ignore, "unique.id=missing")); + + // If we are ignoring unknown options, will see no errors + // for failing or missing + ignore.ignore_unknown_options = true; + ignore.ignore_unsupported_options = false; + ASSERT_OK(c1->ConfigureFromString(ignore, "shared.id=failing")); + ASSERT_OK(c1->ConfigureFromString(ignore, "unique.id=failing")); + ASSERT_OK(c1->ConfigureFromString(ignore, "shared.id=missing")); + ASSERT_OK(c1->ConfigureFromString(ignore, "unique.id=missing")); +} + // Tests that different IDs lead to different objects TEST_F(CustomizableTest, UniqueIdTest) { std::unique_ptr base(new SimpleConfigurable()); diff --git a/util/comparator.cc b/util/comparator.cc index a3c9a8c45..9fbc111b4 100644 --- a/util/comparator.cc +++ b/util/comparator.cc @@ -368,9 +368,10 @@ Status Comparator::CreateFromString(const ConfigOptions& config_options, } else { return status; } - } else if (!opt_map.empty()) { + } else { Comparator* comparator = const_cast(*result); - status = comparator->ConfigureFromMap(config_options, opt_map); + status = + Customizable::ConfigureNewObject(config_options, comparator, opt_map); } } return status; diff --git a/utilities/object_registry_test.cc b/utilities/object_registry_test.cc index 7880703f0..3ab2df6b9 100644 --- a/utilities/object_registry_test.cc +++ b/utilities/object_registry_test.cc @@ -49,30 +49,48 @@ static FactoryFunc test_reg_b = ObjectLibrary::Default()->AddFactory( TEST_F(ObjRegistryTest, Basics) { std::string msg; - std::unique_ptr env_guard; + std::unique_ptr guard; + Env* a_env = nullptr; + auto registry = ObjectRegistry::NewInstance(); - auto res = registry->NewObject("a://test", &env_guard, &msg); - ASSERT_NE(res, nullptr); - ASSERT_EQ(env_guard, nullptr); + ASSERT_NOK(registry->NewStaticObject("c://test", &a_env)); + ASSERT_NOK(registry->NewUniqueObject("c://test", &guard)); + ASSERT_EQ(a_env, nullptr); + ASSERT_EQ(guard, nullptr); + ASSERT_EQ(0, num_a); + ASSERT_EQ(0, num_b); + + ASSERT_OK(registry->NewStaticObject("a://test", &a_env)); + ASSERT_NE(a_env, nullptr); ASSERT_EQ(1, num_a); ASSERT_EQ(0, num_b); - res = registry->NewObject("b://test", &env_guard, &msg); - ASSERT_NE(res, nullptr); - ASSERT_NE(env_guard, nullptr); + ASSERT_OK(registry->NewUniqueObject("b://test", &guard)); + ASSERT_NE(guard, nullptr); ASSERT_EQ(1, num_a); ASSERT_EQ(1, num_b); - res = registry->NewObject("c://test", &env_guard, &msg); - ASSERT_EQ(res, nullptr); - ASSERT_EQ(env_guard, nullptr); + Env* b_env = nullptr; + ASSERT_NOK(registry->NewStaticObject("b://test", &b_env)); + ASSERT_EQ(b_env, nullptr); ASSERT_EQ(1, num_a); - ASSERT_EQ(1, num_b); + ASSERT_EQ(2, num_b); // Created but rejected as not static + + b_env = a_env; + ASSERT_NOK(registry->NewStaticObject("b://test", &b_env)); + ASSERT_EQ(b_env, a_env); + ASSERT_EQ(1, num_a); + ASSERT_EQ(3, num_b); + + b_env = guard.get(); + ASSERT_NOK(registry->NewUniqueObject("a://test", &guard)); + ASSERT_EQ(guard.get(), b_env); // Unchanged + ASSERT_EQ(2, num_a); // Created one but rejected it as not unique + ASSERT_EQ(3, num_b); } TEST_F(ObjRegistryTest, LocalRegistry) { - std::string msg; - std::unique_ptr guard; + Env* env = nullptr; auto registry = ObjectRegistry::NewInstance(); std::shared_ptr library = std::make_shared("local"); @@ -87,14 +105,16 @@ TEST_F(ObjRegistryTest, LocalRegistry) { [](const std::string& /*uri*/, std::unique_ptr* /*guard */, std::string* /* errmsg */) { return Env::Default(); }); - ASSERT_EQ( - ObjectRegistry::NewInstance()->NewObject("test-local", &guard, &msg), - nullptr); - ASSERT_NE( - ObjectRegistry::NewInstance()->NewObject("test-global", &guard, &msg), - nullptr); - ASSERT_NE(registry->NewObject("test-local", &guard, &msg), nullptr); - ASSERT_NE(registry->NewObject("test-global", &guard, &msg), nullptr); + ASSERT_NOK( + ObjectRegistry::NewInstance()->NewStaticObject("test-local", &env)); + ASSERT_EQ(env, nullptr); + ASSERT_OK( + ObjectRegistry::NewInstance()->NewStaticObject("test-global", &env)); + ASSERT_NE(env, nullptr); + ASSERT_OK(registry->NewStaticObject("test-local", &env)); + ASSERT_NE(env, nullptr); + ASSERT_OK(registry->NewStaticObject("test-global", &env)); + ASSERT_NE(env, nullptr); } TEST_F(ObjRegistryTest, CheckShared) { @@ -172,6 +192,36 @@ TEST_F(ObjRegistryTest, CheckUnique) { ASSERT_EQ(unique, nullptr); } +TEST_F(ObjRegistryTest, FailingFactory) { + std::shared_ptr registry = ObjectRegistry::NewInstance(); + std::shared_ptr library = + std::make_shared("failing"); + registry->AddLibrary(library); + library->AddFactory( + "failing", [](const std::string& /*uri*/, + std::unique_ptr* /*guard */, std::string* errmsg) { + *errmsg = "Bad Factory"; + return nullptr; + }); + std::unique_ptr unique; + std::shared_ptr shared; + Env* pointer = nullptr; + Status s; + s = registry->NewUniqueObject("failing", &unique); + ASSERT_TRUE(s.IsInvalidArgument()); + s = registry->NewSharedObject("failing", &shared); + ASSERT_TRUE(s.IsInvalidArgument()); + s = registry->NewStaticObject("failing", &pointer); + ASSERT_TRUE(s.IsInvalidArgument()); + + s = registry->NewUniqueObject("missing", &unique); + ASSERT_TRUE(s.IsNotSupported()); + s = registry->NewSharedObject("missing", &shared); + ASSERT_TRUE(s.IsNotSupported()); + s = registry->NewStaticObject("missing", &pointer); + ASSERT_TRUE(s.IsNotSupported()); +} + TEST_F(ObjRegistryTest, TestRegistryParents) { auto grand = ObjectRegistry::Default(); auto parent = ObjectRegistry::NewInstance(); // parent with a grandparent @@ -194,14 +244,15 @@ TEST_F(ObjRegistryTest, TestRegistryParents) { return guard->get(); }); + Env* env = nullptr; 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); + // a:://* is registered in Default, so they should all work + ASSERT_OK(parent->NewStaticObject("a://test", &env)); + ASSERT_OK(child->NewStaticObject("a://test", &env)); + ASSERT_OK(uncle->NewStaticObject("a://test", &env)); + ASSERT_OK(cousin->NewStaticObject("a://test", &env)); // The parent env is only registered for parent, not uncle, // So parent and child should return success and uncle and cousin should fail