Fix GetOptionsPtr for Wrapped Customizable; Allow null options map (#9213)

Summary: 1. Fix GetOptionsPtr for Wrapped (Inner() != nullptr) Customizable objects. This allows the inner options to be returned via this method. 2. Allow the option type map to be nullptr. This allows objects to be registered as options (for GetOptionsPtr) but not be used by the configuration methods. Added tests as appropriate. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9213 Reviewed By: zhichao-cao Differential Revision: D32718882 Pulled By: mrambacher fbshipit-source-id: 563203d1f006a2629060feb31c5dff9a233e1e83
3 years ago · 7aa31ba4a9
parent 42fef0224f
commit 7aa31ba4a9
4 changed files with 191 additions and 95 deletions
--- a/include/rocksdb/customizable.h
+++ b/include/rocksdb/customizable.h
@ -101,6 +101,20 @@ class Customizable : public Configurable {
    }
  }

+  const void* GetOptionsPtr(const std::string& name) const override {
+    const void* ptr = Configurable::GetOptionsPtr(name);
+    if (ptr != nullptr) {
+      return ptr;
+    } else {
+      const auto inner = Inner();
+      if (inner != nullptr) {
+        return inner->GetOptionsPtr(name);
+      } else {
+        return nullptr;
+      }
+    }
+  }
+
  // Returns the named instance of the Customizable as a T*, or nullptr if not
  // found. This method uses IsInstanceOf/Inner to find the appropriate class
  // instance and then casts it to the expected return type.
--- a/options/configurable.cc
+++ b/options/configurable.cc
@ -43,6 +43,7 @@ Status Configurable::PrepareOptions(const ConfigOptions& opts) {
  Status status = Status::OK();
 #ifndef ROCKSDB_LITE
  for (auto opt_iter : options_) {
+    if (opt_iter.type_map != nullptr) {
      for (auto map_iter : *(opt_iter.type_map)) {
        auto& opt_info = map_iter.second;
        if (!opt_info.IsDeprecated() && !opt_info.IsAlias() &&
@ -56,8 +57,9 @@ Status Configurable::PrepareOptions(const ConfigOptions& opts) {
                return status;
              }
            } else if (!opt_info.CanBeNull()) {
-            status =
-                Status::NotFound("Missing configurable object", map_iter.first);
+              status = Status::NotFound("Missing configurable object",
+                                        map_iter.first);
+            }
          }
        }
      }
@ -74,6 +76,7 @@ Status Configurable::ValidateOptions(const DBOptions& db_opts,
  Status status;
 #ifndef ROCKSDB_LITE
  for (auto opt_iter : options_) {
+    if (opt_iter.type_map != nullptr) {
      for (auto map_iter : *(opt_iter.type_map)) {
        auto& opt_info = map_iter.second;
        if (!opt_info.IsDeprecated() && !opt_info.IsAlias()) {
@ -83,8 +86,8 @@ Status Configurable::ValidateOptions(const DBOptions& db_opts,
            if (config != nullptr) {
              status = config->ValidateOptions(db_opts, cf_opts);
            } else if (!opt_info.CanBeNull()) {
-            status =
-                Status::NotFound("Missing configurable object", map_iter.first);
+              status = Status::NotFound("Missing configurable object",
+                                        map_iter.first);
            }
            if (!status.ok()) {
              return status;
@ -93,6 +96,7 @@ Status Configurable::ValidateOptions(const DBOptions& db_opts,
        }
      }
    }
+  }
 #else
  (void)db_opts;
  (void)cf_opts;
@ -124,6 +128,7 @@ const OptionTypeInfo* ConfigurableHelper::FindOption(
    const std::vector<Configurable::RegisteredOptions>& options,
    const std::string& short_name, std::string* opt_name, void** opt_ptr) {
  for (auto iter : options) {
+    if (iter.type_map != nullptr) {
      const auto opt_info =
          OptionTypeInfo::Find(short_name, *(iter.type_map), opt_name);
      if (opt_info != nullptr) {
@ -131,6 +136,7 @@ const OptionTypeInfo* ConfigurableHelper::FindOption(
        return opt_info;
      }
    }
+  }
  return nullptr;
 }
 #endif  // ROCKSDB_LITE
@ -280,6 +286,7 @@ Status ConfigurableHelper::ConfigureOptions(
  if (!opts_map.empty()) {
 #ifndef ROCKSDB_LITE
    for (const auto& iter : configurable.options_) {
+      if (iter.type_map != nullptr) {
        s = ConfigureSomeOptions(config_options, configurable, *(iter.type_map),
                                 &remaining, iter.opt_ptr);
        if (remaining.empty()) {  // Are there more options left?
@ -288,6 +295,7 @@ Status ConfigurableHelper::ConfigureOptions(
          break;
        }
      }
+    }
 #else
    (void)configurable;
    if (!config_options.ignore_unknown_options) {
@ -573,6 +581,7 @@ Status ConfigurableHelper::SerializeOptions(const ConfigOptions& config_options,
                                            std::string* result) {
  assert(result);
  for (auto const& opt_iter : configurable.options_) {
+    if (opt_iter.type_map != nullptr) {
      for (const auto& map_iter : *(opt_iter.type_map)) {
        const auto& opt_name = map_iter.first;
        const auto& opt_info = map_iter.second;
@ -607,6 +616,7 @@ Status ConfigurableHelper::SerializeOptions(const ConfigOptions& config_options,
        }
      }
    }
+  }
  return Status::OK();
 }
 #endif  // ROCKSDB_LITE
@ -629,6 +639,7 @@ Status ConfigurableHelper::ListOptions(
    const std::string& prefix, std::unordered_set<std::string>* result) {
  Status status;
  for (auto const& opt_iter : configurable.options_) {
+    if (opt_iter.type_map != nullptr) {
      for (const auto& map_iter : *(opt_iter.type_map)) {
        const auto& opt_name = map_iter.first;
        const auto& opt_info = map_iter.second;
@ -643,6 +654,7 @@ Status ConfigurableHelper::ListOptions(
        }
      }
    }
+  }
  return status;
 }
 #endif  // ROCKSDB_LITE
@ -702,7 +714,7 @@ bool ConfigurableHelper::AreEquivalent(const ConfigOptions& config_options,
    if (this_offset != that_offset) {
      if (this_offset == nullptr || that_offset == nullptr) {
        return false;
-      } else {
+      } else if (o.type_map != nullptr) {
        for (const auto& map_iter : *(o.type_map)) {
          const auto& opt_info = map_iter.second;
          if (config_options.IsCheckEnabled(opt_info.GetSanityLevel())) {
--- a/options/configurable_test.cc
+++ b/options/configurable_test.cc
@ -656,6 +656,30 @@ TEST_F(ConfigurableTest, TestNoCompare) {
  ASSERT_TRUE(base->AreEquivalent(config_options_, copy.get(), &mismatch));
  ASSERT_FALSE(copy->AreEquivalent(config_options_, base.get(), &mismatch));
 }
+
+TEST_F(ConfigurableTest, NullOptionMapTest) {
+  std::unique_ptr<Configurable> base;
+  std::unordered_set<std::string> names;
+  std::string str;
+
+  base.reset(
+      SimpleConfigurable::Create("c", TestConfigMode::kDefaultMode, nullptr));
+  ASSERT_NOK(base->ConfigureFromString(config_options_, "int=10"));
+  ASSERT_NOK(base->ConfigureFromString(config_options_, "int=20"));
+  ASSERT_NOK(base->ConfigureOption(config_options_, "int", "20"));
+  ASSERT_NOK(base->GetOption(config_options_, "int", &str));
+  ASSERT_NE(base->GetOptions<TestOptions>("c"), nullptr);
+  ASSERT_OK(base->GetOptionNames(config_options_, &names));
+  ASSERT_EQ(names.size(), 0UL);
+  ASSERT_OK(base->PrepareOptions(config_options_));
+  ASSERT_OK(base->ValidateOptions(DBOptions(), ColumnFamilyOptions()));
+  std::unique_ptr<Configurable> copy;
+  copy.reset(
+      SimpleConfigurable::Create("c", TestConfigMode::kDefaultMode, nullptr));
+  ASSERT_OK(base->GetOptionString(config_options_, &str));
+  ASSERT_OK(copy->ConfigureFromString(config_options_, str));
+  ASSERT_TRUE(base->AreEquivalent(config_options_, copy.get(), &str));
+}
 #endif

 static std::unordered_map<std::string, ConfigTestFactoryFunc> TestFactories = {
--- a/options/customizable_test.cc
+++ b/options/customizable_test.cc
@ -612,11 +612,20 @@ static std::unordered_map<std::string, OptionTypeInfo> inner_option_info = {
 #endif  // ROCKSDB_LITE
 };

+struct InnerOptions {
+  static const char* kName() { return "InnerOptions"; }
+  std::shared_ptr<Customizable> inner;
+};
+
 class InnerCustomizable : public Customizable {
 public:
-  explicit InnerCustomizable(const std::shared_ptr<Customizable>& w)
-      : inner_(w) {}
+  explicit InnerCustomizable(const std::shared_ptr<Customizable>& w) {
+    iopts_.inner = w;
+    RegisterOptions(&iopts_, &inner_option_info);
+  }
  static const char* kClassName() { return "Inner"; }
+  const char* Name() const override { return kClassName(); }
+
  bool IsInstanceOf(const std::string& name) const override {
    if (name == kClassName()) {
      return true;
@ -626,26 +635,51 @@ class InnerCustomizable : public Customizable {
  }

 protected:
-  const Customizable* Inner() const override { return inner_.get(); }
+  const Customizable* Inner() const override { return iopts_.inner.get(); }

 private:
-  std::shared_ptr<Customizable> inner_;
+  InnerOptions iopts_;
+};
+
+struct WrappedOptions1 {
+  static const char* kName() { return "WrappedOptions1"; }
+  int i = 42;
 };

 class WrappedCustomizable1 : public InnerCustomizable {
 public:
  explicit WrappedCustomizable1(const std::shared_ptr<Customizable>& w)
-      : InnerCustomizable(w) {}
+      : InnerCustomizable(w) {
+    RegisterOptions(&wopts_, nullptr);
+  }
  const char* Name() const override { return kClassName(); }
  static const char* kClassName() { return "Wrapped1"; }
+
+ private:
+  WrappedOptions1 wopts_;
 };

+struct WrappedOptions2 {
+  static const char* kName() { return "WrappedOptions2"; }
+  std::string s = "42";
+};
 class WrappedCustomizable2 : public InnerCustomizable {
 public:
  explicit WrappedCustomizable2(const std::shared_ptr<Customizable>& w)
      : InnerCustomizable(w) {}
+  const void* GetOptionsPtr(const std::string& name) const override {
+    if (name == WrappedOptions2::kName()) {
+      return &wopts_;
+    } else {
+      return InnerCustomizable::GetOptionsPtr(name);
+    }
+  }
+
  const char* Name() const override { return kClassName(); }
  static const char* kClassName() { return "Wrapped2"; }
+
+ private:
+  WrappedOptions2 wopts_;
 };
 }  // namespace

@ -677,6 +711,29 @@ TEST_F(CustomizableTest, WrappedInnerTest) {
  ASSERT_EQ(wc2->CheckedCast<TestCustomizable>(), ac.get());
 }

+TEST_F(CustomizableTest, CustomizableInnerTest) {
+  std::shared_ptr<Customizable> c =
+      std::make_shared<InnerCustomizable>(std::make_shared<ACustomizable>("a"));
+  std::shared_ptr<Customizable> wc1 = std::make_shared<WrappedCustomizable1>(c);
+  std::shared_ptr<Customizable> wc2 = std::make_shared<WrappedCustomizable2>(c);
+  auto inner = c->GetOptions<InnerOptions>();
+  ASSERT_NE(inner, nullptr);
+
+  auto aopts = c->GetOptions<AOptions>();
+  ASSERT_NE(aopts, nullptr);
+  ASSERT_EQ(aopts, wc1->GetOptions<AOptions>());
+  ASSERT_EQ(aopts, wc2->GetOptions<AOptions>());
+  auto w1opts = wc1->GetOptions<WrappedOptions1>();
+  ASSERT_NE(w1opts, nullptr);
+  ASSERT_EQ(c->GetOptions<WrappedOptions1>(), nullptr);
+  ASSERT_EQ(wc2->GetOptions<WrappedOptions1>(), nullptr);
+
+  auto w2opts = wc2->GetOptions<WrappedOptions2>();
+  ASSERT_NE(w2opts, nullptr);
+  ASSERT_EQ(c->GetOptions<WrappedOptions2>(), nullptr);
+  ASSERT_EQ(wc1->GetOptions<WrappedOptions2>(), nullptr);
+}
+
 TEST_F(CustomizableTest, CopyObjectTest) {
  class CopyCustomizable : public Customizable {
   public:
@ -714,20 +771,9 @@ TEST_F(CustomizableTest, CopyObjectTest) {
 }

 TEST_F(CustomizableTest, TestStringDepth) {
-  class ShallowCustomizable : public Customizable {
-   public:
-    ShallowCustomizable() {
-      inner_ = std::make_shared<ACustomizable>("a");
-      RegisterOptions("inner", &inner_, &inner_option_info);
-    }
-    static const char* kClassName() { return "shallow"; }
-    const char* Name() const override { return kClassName(); }
-
-   private:
-    std::shared_ptr<TestCustomizable> inner_;
-  };
  ConfigOptions shallow = config_options_;
-  std::unique_ptr<Configurable> c(new ShallowCustomizable());
+  std::unique_ptr<Configurable> c(
+      new InnerCustomizable(std::make_shared<ACustomizable>("a")));
  std::string opt_str;
  shallow.depth = ConfigOptions::Depth::kDepthShallow;
  ASSERT_OK(c->GetOptionString(shallow, &opt_str));