Remove FactoryFunc from LoadXXXObject (facebook#11203)

Summary: The primary purpose of the FactoryFunc was to support LITE mode where the ObjectRegistry was not available. With the removal of LITE mode, the function was no longer required. Note that the MergeOperator had some private classes defined in header files. To gain access to their constructors (and name methods), the class definitions were moved into header files. Pull Request resolved: facebook#11203 Reviewed By: cbi42 Differential Revision: D43160255 Pulled By: pdillinger fbshipit-source-id: f3a465fd5d1a7049b73ecf31e4b8c3762f6dae6c
rockset · Feb 17, 2023 · b6640c3 · b6640c3
1 parent 25e1365
commit b6640c3
Show file tree

Hide file tree

Showing 23 changed files with 320 additions and 360 deletions.
diff --git a/HISTORY.md b/HISTORY.md
@@ -21,6 +21,7 @@
 * Remove deprecated Env::LoadEnv(). Use Env::CreateFromString() instead.
 * Remove deprecated FileSystem::Load(). Use FileSystem::CreateFromString() instead.
 * Removed the deprecated version of these utility functions and the corresponding Java bindings: `LoadOptionsFromFile`, `LoadLatestOptions`, `CheckOptionsCompatibility`.
+* Remove the FactoryFunc from the LoadObject method from the Customizable helper methods.
 
 ### Public API Changes
 * Moved rarely-needed Cache class definition to new advanced_cache.h, and added a CacheWrapper class to advanced_cache.h. Minor changes to SimCache API definitions.

diff --git a/cache/cache.cc b/cache/cache.cc
@@ -87,8 +87,7 @@ Status SecondaryCache::CreateFromString(
     }
     return status;
   } else {
-    return LoadSharedObject<SecondaryCache>(config_options, value, nullptr,
-                                            result);
+    return LoadSharedObject<SecondaryCache>(config_options, value, result);
   }
 }
 

diff --git a/db/compaction/sst_partitioner.cc b/db/compaction/sst_partitioner.cc
@@ -78,7 +78,6 @@ Status SstPartitionerFactory::CreateFromString(
   std::call_once(once, [&]() {
     RegisterSstPartitionerFactories(*(ObjectLibrary::Default().get()), "");
   });
-  return LoadSharedObject<SstPartitionerFactory>(options, value, nullptr,
-                                                 result);
+  return LoadSharedObject<SstPartitionerFactory>(options, value, result);
 }
 }  // namespace ROCKSDB_NAMESPACE
diff --git a/db/event_helpers.cc b/db/event_helpers.cc
@@ -13,7 +13,7 @@ namespace ROCKSDB_NAMESPACE {
 Status EventListener::CreateFromString(const ConfigOptions& config_options,
                                        const std::string& id,
                                        std::shared_ptr<EventListener>* result) {
-  return LoadSharedObject<EventListener>(config_options, id, nullptr, result);
+  return LoadSharedObject<EventListener>(config_options, id, result);
 }
 
 namespace {

diff --git a/env/env.cc b/env/env.cc
@@ -641,7 +641,7 @@ Status Env::CreateFromString(const ConfigOptions& config_options,
   } else {
     RegisterSystemEnvs();
     Env* env = *result;
-    Status s = LoadStaticObject<Env>(config_options, value, nullptr, &env);
+    Status s = LoadStaticObject<Env>(config_options, value, &env);
     if (s.ok()) {
       *result = env;
     }
@@ -1227,8 +1227,7 @@ Status SystemClock::CreateFromString(const ConfigOptions& config_options,
     std::call_once(once, [&]() {
       RegisterBuiltinSystemClocks(*(ObjectLibrary::Default().get()), "");
     });
-    return LoadSharedObject<SystemClock>(config_options, value, nullptr,
-                                         result);
+    return LoadSharedObject<SystemClock>(config_options, value, result);
   }
 }
 }  // namespace ROCKSDB_NAMESPACE
diff --git a/env/env_encryption.cc b/env/env_encryption.cc
@@ -1332,15 +1332,14 @@ Status BlockCipher::CreateFromString(const ConfigOptions& config_options,
                                      const std::string& value,
                                      std::shared_ptr<BlockCipher>* result) {
   RegisterEncryptionBuiltins();
-  return LoadSharedObject<BlockCipher>(config_options, value, nullptr, result);
+  return LoadSharedObject<BlockCipher>(config_options, value, result);
 }
 
 Status EncryptionProvider::CreateFromString(
     const ConfigOptions& config_options, const std::string& value,
     std::shared_ptr<EncryptionProvider>* result) {
   RegisterEncryptionBuiltins();
-  return LoadSharedObject<EncryptionProvider>(config_options, value, nullptr,
-                                              result);
+  return LoadSharedObject<EncryptionProvider>(config_options, value, result);
 }
 
 

diff --git a/env/file_system.cc b/env/file_system.cc
@@ -91,7 +91,7 @@ Status FileSystem::CreateFromString(const ConfigOptions& config_options,
     std::call_once(once, [&]() {
       RegisterBuiltinFileSystems(*(ObjectLibrary::Default().get()), "");
     });
-    return LoadSharedObject<FileSystem>(config_options, value, nullptr, result);
+    return LoadSharedObject<FileSystem>(config_options, value, result);
   }
 }
 

diff --git a/include/rocksdb/utilities/customizable_util.h b/include/rocksdb/utilities/customizable_util.h
@@ -13,7 +13,6 @@
 // for more information on how to develop and use customizable objects
 
 #pragma once
-#include <functional>
 #include <memory>
 #include <unordered_map>
 
@@ -24,24 +23,6 @@
 #include "rocksdb/utilities/object_registry.h"
 
 namespace ROCKSDB_NAMESPACE {
-// The FactoryFunc functions are used to create a new customizable object
-// without going through the ObjectRegistry.  This methodology is especially
-// useful in LITE mode, where there is no ObjectRegistry.  The methods take
-// in an ID of the object to create and a pointer to store the created object.
-// If the factory successfully recognized the input ID, the method should return
-// success; otherwise false should be returned.  On success, the object
-// parameter contains the new object.
-template <typename T>
-using SharedFactoryFunc =
-    std::function<bool(const std::string&, std::shared_ptr<T>*)>;
-
-template <typename T>
-using UniqueFactoryFunc =
-    std::function<bool(const std::string&, std::unique_ptr<T>*)>;
-
-template <typename T>
-using StaticFactoryFunc = std::function<bool(const std::string&, T**)>;
-
 // Creates a new shared customizable instance object based on the
 // input parameters using the object registry.
 //
@@ -156,12 +137,10 @@ static Status NewManagedObject(
 // handled
 // @param value Either the simple name of the instance to create, or a set of
 // name-value pairs to create and initailize the object
-// @param func  Optional function to call to attempt to create an instance
 // @param result The newly created instance.
 template <typename T>
 static Status LoadSharedObject(const ConfigOptions& config_options,
                                const std::string& value,
-                               const SharedFactoryFunc<T>& func,
                                std::shared_ptr<T>* result) {
   std::string id;
   std::unordered_map<std::string, std::string> opt_map;
@@ -170,12 +149,8 @@ static Status LoadSharedObject(const ConfigOptions& config_options,
                                               value, &id, &opt_map);
   if (!status.ok()) {  // GetOptionsMap failed
     return status;
-  } else if (func == nullptr ||
-             !func(id, result)) {  // No factory, or it failed
-    return NewSharedObject(config_options, id, opt_map, result);
   } else {
-    return Customizable::ConfigureNewObject(config_options, result->get(),
-                                            opt_map);
+    return NewSharedObject(config_options, id, opt_map, result);
   }
 }
 
@@ -204,7 +179,6 @@ static Status LoadSharedObject(const ConfigOptions& config_options,
 // handled
 // @param value Either the simple name of the instance to create, or a set of
 // name-value pairs to create and initailize the object
-// @param func  Optional function to call to attempt to create an instance
 // @param result The newly created instance.
 template <typename T>
 static Status LoadManagedObject(const ConfigOptions& config_options,
@@ -270,25 +244,19 @@ static Status NewUniqueObject(
 // handled
 // @param value Either the simple name of the instance to create, or a set of
 // name-value pairs to create and initailize the object
-// @param func  Optional function to call to attempt to create an instance
 // @param result The newly created instance.
 template <typename T>
 static Status LoadUniqueObject(const ConfigOptions& config_options,
                                const std::string& value,
-                               const UniqueFactoryFunc<T>& func,
                                std::unique_ptr<T>* result) {
   std::string id;
   std::unordered_map<std::string, std::string> opt_map;
   Status status = Customizable::GetOptionsMap(config_options, result->get(),
                                               value, &id, &opt_map);
   if (!status.ok()) {  // GetOptionsMap failed
     return status;
-  } else if (func == nullptr ||
-             !func(id, result)) {  // No factory, or it failed
-    return NewUniqueObject(config_options, id, opt_map, result);
   } else {
-    return Customizable::ConfigureNewObject(config_options, result->get(),
-                                            opt_map);
+    return NewUniqueObject(config_options, id, opt_map, result);
   }
 }
 
@@ -337,23 +305,18 @@ static Status NewStaticObject(
 // handled
 // @param value Either the simple name of the instance to create, or a set of
 // name-value pairs to create and initailize the object
-// @param func  Optional function to call to attempt to create an instance
 // @param result The newly created instance.
 template <typename T>
 static Status LoadStaticObject(const ConfigOptions& config_options,
-                               const std::string& value,
-                               const StaticFactoryFunc<T>& func, T** result) {
+                               const std::string& value, T** result) {
   std::string id;
   std::unordered_map<std::string, std::string> opt_map;
   Status status = Customizable::GetOptionsMap(config_options, *result, value,
                                               &id, &opt_map);
   if (!status.ok()) {  // GetOptionsMap failed
     return status;
-  } else if (func == nullptr ||
-             !func(id, result)) {  // No factory, or it failed
-    return NewStaticObject(config_options, id, opt_map, result);
   } else {
-    return Customizable::ConfigureNewObject(config_options, *result, opt_map);
+    return NewStaticObject(config_options, id, opt_map, result);
   }
 }
 }  // namespace ROCKSDB_NAMESPACE
diff --git a/monitoring/statistics.cc b/monitoring/statistics.cc
@@ -300,7 +300,7 @@ Status Statistics::CreateFromString(const ConfigOptions& config_options,
   } else if (id == kNullptrString) {
     result->reset();
   } else {
-    s = LoadSharedObject<Statistics>(config_options, id, nullptr, result);
+    s = LoadSharedObject<Statistics>(config_options, id, result);
   }
   return s;
 }

diff --git a/options/customizable_test.cc b/options/customizable_test.cc
@@ -160,19 +160,6 @@ class BCustomizable : public TestCustomizable {
   BOptions opts_;
 };
 
-static bool LoadSharedB(const std::string& id,
-                        std::shared_ptr<TestCustomizable>* result) {
-  if (id == "B") {
-    result->reset(new BCustomizable(id));
-    return true;
-  } else if (id.empty()) {
-    result->reset();
-    return true;
-  } else {
-    return false;
-  }
-}
-
 static int A_count = 0;
 static int RegisterCustomTestObjects(ObjectLibrary& library,
                                      const std::string& /*arg*/) {
@@ -184,6 +171,12 @@ static int RegisterCustomTestObjects(ObjectLibrary& library,
         A_count++;
         return guard->get();
       });
+  library.AddFactory<TestCustomizable>(
+      "B", [](const std::string& name, std::unique_ptr<TestCustomizable>* guard,
+              std::string* /* msg */) {
+        guard->reset(new BCustomizable(name));
+        return guard->get();
+      });
 
   library.AddFactory<TestCustomizable>(
       "S", [](const std::string& name,
@@ -252,46 +245,19 @@ static void GetMapFromProperties(
 Status TestCustomizable::CreateFromString(
     const ConfigOptions& config_options, const std::string& value,
     std::shared_ptr<TestCustomizable>* result) {
-  return LoadSharedObject<TestCustomizable>(config_options, value, LoadSharedB,
-                                            result);
+  return LoadSharedObject<TestCustomizable>(config_options, value, result);
 }
 
 Status TestCustomizable::CreateFromString(
     const ConfigOptions& config_options, const std::string& value,
     std::unique_ptr<TestCustomizable>* result) {
-  return LoadUniqueObject<TestCustomizable>(
-      config_options, value,
-      [](const std::string& id, std::unique_ptr<TestCustomizable>* u) {
-        if (id == "B") {
-          u->reset(new BCustomizable(id));
-          return true;
-        } else if (id.empty()) {
-          u->reset();
-          return true;
-        } else {
-          return false;
-        }
-      },
-      result);
+  return LoadUniqueObject<TestCustomizable>(config_options, value, result);
 }
 
 Status TestCustomizable::CreateFromString(const ConfigOptions& config_options,
                                           const std::string& value,
                                           TestCustomizable** result) {
-  return LoadStaticObject<TestCustomizable>(
-      config_options, value,
-      [](const std::string& id, TestCustomizable** ptr) {
-        if (id == "B") {
-          *ptr = new BCustomizable(id);
-          return true;
-        } else if (id.empty()) {
-          *ptr = nullptr;
-          return true;
-        } else {
-          return false;
-        }
-      },
-      result);
+  return LoadStaticObject<TestCustomizable>(config_options, value, result);
 }
 
 class CustomizableTest : public testing::Test {
@@ -980,70 +946,34 @@ TEST_F(CustomizableTest, IgnoreUnknownObjects) {
   std::unique_ptr<TestCustomizable> unique;
   TestCustomizable* pointer = nullptr;
   ignore.ignore_unsupported_options = false;
-  ASSERT_NOK(
-      LoadSharedObject<TestCustomizable>(ignore, "Unknown", nullptr, &shared));
-  ASSERT_NOK(
-      LoadUniqueObject<TestCustomizable>(ignore, "Unknown", nullptr, &unique));
-  ASSERT_NOK(
-      LoadStaticObject<TestCustomizable>(ignore, "Unknown", nullptr, &pointer));
+  ASSERT_NOK(LoadSharedObject<TestCustomizable>(ignore, "Unknown", &shared));
+  ASSERT_NOK(LoadUniqueObject<TestCustomizable>(ignore, "Unknown", &unique));
+  ASSERT_NOK(LoadStaticObject<TestCustomizable>(ignore, "Unknown", &pointer));
   ASSERT_EQ(shared.get(), nullptr);
   ASSERT_EQ(unique.get(), nullptr);
   ASSERT_EQ(pointer, nullptr);
   ignore.ignore_unsupported_options = true;
-  ASSERT_OK(
-      LoadSharedObject<TestCustomizable>(ignore, "Unknown", nullptr, &shared));
-  ASSERT_OK(
-      LoadUniqueObject<TestCustomizable>(ignore, "Unknown", nullptr, &unique));
-  ASSERT_OK(
-      LoadStaticObject<TestCustomizable>(ignore, "Unknown", nullptr, &pointer));
+  ASSERT_OK(LoadSharedObject<TestCustomizable>(ignore, "Unknown", &shared));
+  ASSERT_OK(LoadUniqueObject<TestCustomizable>(ignore, "Unknown", &unique));
+  ASSERT_OK(LoadStaticObject<TestCustomizable>(ignore, "Unknown", &pointer));
   ASSERT_EQ(shared.get(), nullptr);
   ASSERT_EQ(unique.get(), nullptr);
   ASSERT_EQ(pointer, nullptr);
-  ASSERT_OK(LoadSharedObject<TestCustomizable>(ignore, "id=Unknown", nullptr,
-                                               &shared));
-  ASSERT_OK(LoadUniqueObject<TestCustomizable>(ignore, "id=Unknown", nullptr,
-                                               &unique));
-  ASSERT_OK(LoadStaticObject<TestCustomizable>(ignore, "id=Unknown", nullptr,
-                                               &pointer));
+  ASSERT_OK(LoadSharedObject<TestCustomizable>(ignore, "id=Unknown", &shared));
+  ASSERT_OK(LoadUniqueObject<TestCustomizable>(ignore, "id=Unknown", &unique));
+  ASSERT_OK(LoadStaticObject<TestCustomizable>(ignore, "id=Unknown", &pointer));
   ASSERT_EQ(shared.get(), nullptr);
   ASSERT_EQ(unique.get(), nullptr);
   ASSERT_EQ(pointer, nullptr);
   ASSERT_OK(LoadSharedObject<TestCustomizable>(ignore, "id=Unknown;option=bad",
-                                               nullptr, &shared));
+                                               &shared));
   ASSERT_OK(LoadUniqueObject<TestCustomizable>(ignore, "id=Unknown;option=bad",
-                                               nullptr, &unique));
+                                               &unique));
   ASSERT_OK(LoadStaticObject<TestCustomizable>(ignore, "id=Unknown;option=bad",
-                                               nullptr, &pointer));
-  ASSERT_EQ(shared.get(), nullptr);
-  ASSERT_EQ(unique.get(), nullptr);
-  ASSERT_EQ(pointer, nullptr);
-}
-
-TEST_F(CustomizableTest, FactoryFunctionTest) {
-  std::shared_ptr<TestCustomizable> shared;
-  std::unique_ptr<TestCustomizable> unique;
-  TestCustomizable* pointer = nullptr;
-  ConfigOptions ignore = config_options_;
-  ignore.ignore_unsupported_options = false;
-  ASSERT_OK(TestCustomizable::CreateFromString(ignore, "B", &shared));
-  ASSERT_OK(TestCustomizable::CreateFromString(ignore, "B", &unique));
-  ASSERT_OK(TestCustomizable::CreateFromString(ignore, "B", &pointer));
-  ASSERT_NE(shared.get(), nullptr);
-  ASSERT_NE(unique.get(), nullptr);
-  ASSERT_NE(pointer, nullptr);
-  delete pointer;
-  pointer = nullptr;
-  ASSERT_OK(TestCustomizable::CreateFromString(ignore, "id=", &shared));
-  ASSERT_OK(TestCustomizable::CreateFromString(ignore, "id=", &unique));
-  ASSERT_OK(TestCustomizable::CreateFromString(ignore, "id=", &pointer));
+                                               &pointer));
   ASSERT_EQ(shared.get(), nullptr);
   ASSERT_EQ(unique.get(), nullptr);
   ASSERT_EQ(pointer, nullptr);
-  ASSERT_NOK(TestCustomizable::CreateFromString(ignore, "option=bad", &shared));
-  ASSERT_NOK(TestCustomizable::CreateFromString(ignore, "option=bad", &unique));
-  ASSERT_NOK(
-      TestCustomizable::CreateFromString(ignore, "option=bad", &pointer));
-  ASSERT_EQ(pointer, nullptr);
 }
 
 TEST_F(CustomizableTest, URLFactoryTest) {