Fix PrepareOptions for Customizable Classes (#8468)

Summary:
Added the Customizable::ConfigureNewObject method.  The method will configure the object if options are found and invoke PrepareOptions if the flag is set properly.

Added tests to test that PrepareOptions is properly called and to test if PrepareOptions fails.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/8468

Reviewed By: zhichao-cao

Differential Revision: D29494703

Pulled By: mrambacher

fbshipit-source-id: d5767dee5d7a98620ac66190262101cd0aa9d2b7

include/rocksdb/customizable.h

@@ -162,6 +162,17 @@ class Customizable : public Configurable {
       const std::string& opt_value, std::string* id,
       std::unordered_map<std::string, std::string>* 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<std::string, std::string>& 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.

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

options/configurable.cc

@@ -39,6 +39,7 @@ 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)) {
@@ -53,17 +54,19 @@ Status Configurable::PrepareOptions(const ConfigOptions& 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;
     }
+  }
   return status;
 }
 
@@ -158,17 +161,26 @@ Status Configurable::ConfigureOptions(
     const std::unordered_map<std::string, std::string>& opts_map,
     std::unordered_map<std::string, std::string>* unused) {
   std::string curr_opts;
+  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.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
+      // If we are not ignoring unused, get the defaults in case we need to
-    ConfigOptions copy = config_options;
+      // 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();
   }

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<std::string, std::string>& 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

options/customizable_test.cc

@@ -217,8 +217,8 @@ const FactoryFunc<TestCustomizable>& s_func =
 #endif  // ROCKSDB_LITE
 
 struct SimpleOptions {
+  static const char* kName() { return "simple"; }
   bool b = true;
-  bool is_mutable = true;
   std::unique_ptr<TestCustomizable> cu;
   std::shared_ptr<TestCustomizable> cs;
   TestCustomizable* cp = nullptr;
@@ -246,28 +246,18 @@ class SimpleConfigurable : public Configurable {
   SimpleOptions simple_;
 
  public:
-  SimpleConfigurable() {
+  SimpleConfigurable() { RegisterOptions(&simple_, &simple_option_info); }
-    RegisterOptions("simple", &simple_, &simple_option_info);
-  }
 
   explicit SimpleConfigurable(
       const std::unordered_map<std::string, OptionTypeInfo>* map) {
-    RegisterOptions("simple", &simple_, map);
+    RegisterOptions(&simple_, map);
-  }
-
-  bool IsPrepared() const override {
-    if (simple_.is_mutable) {
-      return false;
-    } else {
-      return Configurable::IsPrepared();
-    }
   }
-
- 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> configurable(new SimpleConfigurable());
-  SimpleOptions* simple = configurable->GetOptions<SimpleOptions>("simple");
+  SimpleOptions* simple = configurable->GetOptions<SimpleOptions>();
   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> configurable(new SimpleConfigurable());
   ASSERT_OK(configurable->ConfigureFromMap(config_options_, opt_map));
-  SimpleOptions* simple = configurable->GetOptions<SimpleOptions>("simple");
+  SimpleOptions* simple = configurable->GetOptions<SimpleOptions>();
   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> configurable(new SimpleConfigurable());
   ASSERT_OK(configurable->ConfigureFromMap(config_options_, opt_map));
-  SimpleOptions* simple = configurable->GetOptions<SimpleOptions>("simple");
+  SimpleOptions* simple = configurable->GetOptions<SimpleOptions>();
   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> configurable(new SimpleConfigurable());
   ASSERT_OK(configurable->ConfigureFromMap(config_options_, opt_map));
-  SimpleOptions* simple = configurable->GetOptions<SimpleOptions>("simple");
+  SimpleOptions* simple = configurable->GetOptions<SimpleOptions>();
   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<Configurable> c1(new SimpleConfigurable());
   std::unique_ptr<Configurable> 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<SimpleOptions>("simple");
+  SimpleOptions* simple = c1->GetOptions<SimpleOptions>();
   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<Configurable> base(new SimpleConfigurable());
   ASSERT_OK(base->ConfigureFromString(config_options_,
                                       "unique={id=A_1;int=1;bool=true}"));
-  SimpleOptions* simple = base->GetOptions<SimpleOptions>("simple");
+  SimpleOptions* simple = base->GetOptions<SimpleOptions>();
   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<ACustomizable>(), nullptr);
 }
 
+TEST_F(CustomizableTest, PrepareOptionsTest) {
+  static std::unordered_map<std::string, OptionTypeInfo> 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<TestCustomizable>(
+      "P",
+      [](const std::string& /*name*/, std::unique_ptr<TestCustomizable>* guard,
+         std::string* /* msg */) {
+        guard->reset(new PrepareCustomizable());
+        return guard->get();
+      });
+
+  std::unique_ptr<Configurable> 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<SimpleOptions>();
+  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<SimpleOptions>();
+  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<SimpleOptions>();
+  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<std::string, OptionTypeInfo> 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<SimpleOptions>("simple");
+  SimpleOptions* simple = base->GetOptions<SimpleOptions>();
   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<std::string, OptionTypeInfo> immutable_option_info =
       {{"immutable",
         OptionTypeInfo::AsCustomSharedPtr<TestCustomizable>(
-            0, OptionVerificationType::kNormal, OptionTypeFlags::kNone)}};
+            0, OptionVerificationType::kNormal, OptionTypeFlags::kAllowNull)}};
 
   class MutableCustomizable : public Customizable {
    private: