Allow unregistered options to be ignored in DBOptions from files (#9045)

Summary:
Adds changes to DBOptions (comparable to ColumnFamilyOptions) to allow some option values to be ignored on rehydration from the Options file.  This is necessary for some customizable classes that were not registered with the ObjectRegistry but are saved/restored from the Options file.

All tests pass.  Will run check_format_compatible.sh shortly.

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

Reviewed By: zhichao-cao

Differential Revision: D31761664

Pulled By: mrambacher

fbshipit-source-id: 300c2251639cce2b223481c3bb2a63877b1f3766

options/customizable_test.cc

@@ -1463,7 +1463,6 @@ class LoadCustomizableTest : public testing::Test {
 };
 
 TEST_F(LoadCustomizableTest, LoadTableFactoryTest) {
-  ColumnFamilyOptions cf_opts;
   std::shared_ptr<TableFactory> factory;
   ASSERT_NOK(TableFactory::CreateFromString(
       config_options_, mock::MockTableFactory::kClassName(), &factory));
@@ -1474,10 +1473,10 @@ TEST_F(LoadCustomizableTest, LoadTableFactoryTest) {
 #ifndef ROCKSDB_LITE
   std::string opts_str = "table_factory=";
   ASSERT_OK(GetColumnFamilyOptionsFromString(
-      config_options_, ColumnFamilyOptions(),
+      config_options_, cf_opts_,
-      opts_str + TableFactory::kBlockBasedTableName(), &cf_opts));
+      opts_str + TableFactory::kBlockBasedTableName(), &cf_opts_));
-  ASSERT_NE(cf_opts.table_factory.get(), nullptr);
+  ASSERT_NE(cf_opts_.table_factory.get(), nullptr);
-  ASSERT_STREQ(cf_opts.table_factory->Name(),
+  ASSERT_STREQ(cf_opts_.table_factory->Name(),
                TableFactory::kBlockBasedTableName());
 #endif  // ROCKSDB_LITE
   if (RegisterTests("Test")) {
@@ -1487,10 +1486,10 @@ TEST_F(LoadCustomizableTest, LoadTableFactoryTest) {
     ASSERT_STREQ(factory->Name(), mock::MockTableFactory::kClassName());
 #ifndef ROCKSDB_LITE
     ASSERT_OK(GetColumnFamilyOptionsFromString(
-        config_options_, ColumnFamilyOptions(),
+        config_options_, cf_opts_,
-        opts_str + mock::MockTableFactory::kClassName(), &cf_opts));
+        opts_str + mock::MockTableFactory::kClassName(), &cf_opts_));
-    ASSERT_NE(cf_opts.table_factory.get(), nullptr);
+    ASSERT_NE(cf_opts_.table_factory.get(), nullptr);
-    ASSERT_STREQ(cf_opts.table_factory->Name(),
+    ASSERT_STREQ(cf_opts_.table_factory->Name(),
                  mock::MockTableFactory::kClassName());
 #endif  // ROCKSDB_LITE
   }
@@ -1763,12 +1762,12 @@ TEST_F(LoadCustomizableTest, LoadEncryptionProviderTest) {
       EncryptionProvider::CreateFromString(config_options_, "CTR", &result));
   ASSERT_NE(result, nullptr);
   ASSERT_STREQ(result->Name(), "CTR");
-  ASSERT_NOK(result->ValidateOptions(DBOptions(), ColumnFamilyOptions()));
+  ASSERT_NOK(result->ValidateOptions(db_opts_, cf_opts_));
   ASSERT_OK(EncryptionProvider::CreateFromString(config_options_, "CTR://test",
                                                  &result));
   ASSERT_NE(result, nullptr);
   ASSERT_STREQ(result->Name(), "CTR");
-  ASSERT_OK(result->ValidateOptions(DBOptions(), ColumnFamilyOptions()));
+  ASSERT_OK(result->ValidateOptions(db_opts_, cf_opts_));
 
   if (RegisterTests("Test")) {
     ASSERT_OK(
@@ -1779,7 +1778,7 @@ TEST_F(LoadCustomizableTest, LoadEncryptionProviderTest) {
                                                    "Mock://test", &result));
     ASSERT_NE(result, nullptr);
     ASSERT_STREQ(result->Name(), "Mock");
-    ASSERT_OK(result->ValidateOptions(DBOptions(), ColumnFamilyOptions()));
+    ASSERT_OK(result->ValidateOptions(db_opts_, cf_opts_));
   }
 }
 

options/db_options.cc

@@ -452,7 +452,8 @@ static std::unordered_map<std::string, OptionTypeInfo>
         {"file_checksum_gen_factory",
          OptionTypeInfo::AsCustomSharedPtr<FileChecksumGenFactory>(
              offsetof(struct ImmutableDBOptions, file_checksum_gen_factory),
-             OptionVerificationType::kByName, OptionTypeFlags::kAllowNull)},
+             OptionVerificationType::kByNameAllowFromNull,
+             OptionTypeFlags::kAllowNull)},
         {"statistics",
          OptionTypeInfo::AsCustomSharedPtr<Statistics>(
              // Statistics should not be compared and can be null
@@ -529,19 +530,63 @@ const std::string OptionsHelper::kDBOptionsName = "DBOptions";
 
 class MutableDBConfigurable : public Configurable {
  public:
-  explicit MutableDBConfigurable(const MutableDBOptions& mdb) {
+  explicit MutableDBConfigurable(
-    mutable_ = mdb;
+      const MutableDBOptions& mdb,
+      const std::unordered_map<std::string, std::string>* map = nullptr)
+      : mutable_(mdb), opt_map_(map) {
     RegisterOptions(&mutable_, &db_mutable_options_type_info);
   }
 
+  bool OptionsAreEqual(const ConfigOptions& config_options,
+                       const OptionTypeInfo& opt_info,
+                       const std::string& opt_name, const void* const this_ptr,
+                       const void* const that_ptr,
+                       std::string* mismatch) const override {
+    bool equals = opt_info.AreEqual(config_options, opt_name, this_ptr,
+                                    that_ptr, mismatch);
+    if (!equals && opt_info.IsByName()) {
+      if (opt_map_ == nullptr) {
+        equals = true;
+      } else {
+        const auto& iter = opt_map_->find(opt_name);
+        if (iter == opt_map_->end()) {
+          equals = true;
+        } else {
+          equals = opt_info.AreEqualByName(config_options, opt_name, this_ptr,
+                                           iter->second);
+        }
+      }
+      if (equals) {  // False alarm, clear mismatch
+        *mismatch = "";
+      }
+    }
+    if (equals && opt_info.IsConfigurable() && opt_map_ != nullptr) {
+      const auto* this_config = opt_info.AsRawPointer<Configurable>(this_ptr);
+      if (this_config == nullptr) {
+        const auto& iter = opt_map_->find(opt_name);
+        // If the name exists in the map and is not empty/null,
+        // then the this_config should be set.
+        if (iter != opt_map_->end() && !iter->second.empty() &&
+            iter->second != kNullptrString) {
+          *mismatch = opt_name;
+          equals = false;
+        }
+      }
+    }
+    return equals;
+  }
+
  protected:
   MutableDBOptions mutable_;
+  const std::unordered_map<std::string, std::string>* opt_map_;
 };
 
 class DBOptionsConfigurable : public MutableDBConfigurable {
  public:
-  explicit DBOptionsConfigurable(const DBOptions& opts)
+  explicit DBOptionsConfigurable(
-      : MutableDBConfigurable(MutableDBOptions(opts)), db_options_(opts) {
+      const DBOptions& opts,
+      const std::unordered_map<std::string, std::string>* map = nullptr)
+      : MutableDBConfigurable(MutableDBOptions(opts), map), db_options_(opts) {
     // The ImmutableDBOptions currently requires the env to be non-null.  Make
     // sure it is
     if (opts.env != nullptr) {
@@ -585,8 +630,10 @@ std::unique_ptr<Configurable> DBOptionsAsConfigurable(
   std::unique_ptr<Configurable> ptr(new MutableDBConfigurable(opts));
   return ptr;
 }
-std::unique_ptr<Configurable> DBOptionsAsConfigurable(const DBOptions& opts) {
+std::unique_ptr<Configurable> DBOptionsAsConfigurable(
-  std::unique_ptr<Configurable> ptr(new DBOptionsConfigurable(opts));
+    const DBOptions& opts,
+    const std::unordered_map<std::string, std::string>* opt_map) {
+  std::unique_ptr<Configurable> ptr(new DBOptionsConfigurable(opts, opt_map));
   return ptr;
 }
 #endif  // ROCKSDB_LITE

options/options_helper.h

@@ -47,7 +47,9 @@ void UpdateColumnFamilyOptions(const MutableCFOptions& moptions,
 #ifndef ROCKSDB_LITE
 std::unique_ptr<Configurable> DBOptionsAsConfigurable(
     const MutableDBOptions& opts);
-std::unique_ptr<Configurable> DBOptionsAsConfigurable(const DBOptions& opts);
+std::unique_ptr<Configurable> DBOptionsAsConfigurable(
+    const DBOptions& opts,
+    const std::unordered_map<std::string, std::string>* opt_map = nullptr);
 std::unique_ptr<Configurable> CFOptionsAsConfigurable(
     const MutableCFOptions& opts);
 std::unique_ptr<Configurable> CFOptionsAsConfigurable(

options/options_parser.cc

@@ -553,6 +553,12 @@ Status RocksDBOptionsParser::VerifyRocksDBOptionsFromFile(
   ConfigOptions config_options = config_options_in;
   config_options.invoke_prepare_options =
       false;  // No need to do a prepare for verify
+  if (config_options.sanity_level < ConfigOptions::kSanityLevelExactMatch) {
+    // If we are not doing an exact comparison, we should ignore
+    // unsupported options, as they may cause the Parse to fail
+    // (if the ObjectRegistry is not initialized)
+    config_options.ignore_unsupported_options = true;
+  }
   Status s = parser.Parse(config_options, file_name, fs);
   if (!s.ok()) {
     return s;
@@ -622,9 +628,9 @@ Status RocksDBOptionsParser::VerifyRocksDBOptionsFromFile(
 Status RocksDBOptionsParser::VerifyDBOptions(
     const ConfigOptions& config_options, const DBOptions& base_opt,
     const DBOptions& file_opt,
-    const std::unordered_map<std::string, std::string>* /*opt_map*/) {
+    const std::unordered_map<std::string, std::string>* opt_map) {
-  auto base_config = DBOptionsAsConfigurable(base_opt);
+  auto base_config = DBOptionsAsConfigurable(base_opt, opt_map);
-  auto file_config = DBOptionsAsConfigurable(file_opt);
+  auto file_config = DBOptionsAsConfigurable(file_opt, opt_map);
   std::string mismatch;
   if (!base_config->AreEquivalent(config_options, file_config.get(),
                                   &mismatch)) {

options/options_test.cc

@@ -3710,37 +3710,86 @@ TEST_F(OptionsParserTest, DifferentDefault) {
   ASSERT_EQ(5000, small_opts.max_open_files);
 }
 
-class OptionsSanityCheckTest : public OptionsParserTest {
+class OptionsSanityCheckTest : public OptionsParserTest,
+                               public ::testing::WithParamInterface<bool> {
+ protected:
+  ConfigOptions config_options_;
+
  public:
-  OptionsSanityCheckTest() {}
+  OptionsSanityCheckTest() {
+    config_options_.ignore_unknown_options = false;
+    config_options_.ignore_unsupported_options = GetParam();
+    config_options_.input_strings_escaped = true;
+  }
 
  protected:
-  Status SanityCheckCFOptions(const ColumnFamilyOptions& cf_opts,
+  Status SanityCheckOptions(const DBOptions& db_opts,
-                              ConfigOptions::SanityLevel level,
+                            const ColumnFamilyOptions& cf_opts,
-                              bool input_strings_escaped = true) {
+                            ConfigOptions::SanityLevel level) {
-    ConfigOptions config_options;
+    config_options_.sanity_level = level;
-    config_options.sanity_level = level;
-    config_options.ignore_unknown_options = false;
-    config_options.input_strings_escaped = input_strings_escaped;
-
     return RocksDBOptionsParser::VerifyRocksDBOptionsFromFile(
-        config_options, DBOptions(), {"default"}, {cf_opts}, kOptionsFileName,
+        config_options_, db_opts, {"default"}, {cf_opts}, kOptionsFileName,
         fs_.get());
   }
 
-  Status PersistCFOptions(const ColumnFamilyOptions& cf_opts) {
+  Status SanityCheckCFOptions(const ColumnFamilyOptions& cf_opts,
+                              ConfigOptions::SanityLevel level) {
+    return SanityCheckOptions(DBOptions(), cf_opts, level);
+  }
+
+  void SanityCheckCFOptions(const ColumnFamilyOptions& opts, bool exact) {
+    ASSERT_OK(SanityCheckCFOptions(
+        opts, ConfigOptions::kSanityLevelLooselyCompatible));
+    ASSERT_OK(SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelNone));
+    if (exact) {
+      ASSERT_OK(
+          SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch));
+    } else {
+      ASSERT_NOK(
+          SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch));
+    }
+  }
+
+  Status SanityCheckDBOptions(const DBOptions& db_opts,
+                              ConfigOptions::SanityLevel level) {
+    return SanityCheckOptions(db_opts, ColumnFamilyOptions(), level);
+  }
+
+  void SanityCheckDBOptions(const DBOptions& opts, bool exact) {
+    ASSERT_OK(SanityCheckDBOptions(
+        opts, ConfigOptions::kSanityLevelLooselyCompatible));
+    ASSERT_OK(SanityCheckDBOptions(opts, ConfigOptions::kSanityLevelNone));
+    if (exact) {
+      ASSERT_OK(
+          SanityCheckDBOptions(opts, ConfigOptions::kSanityLevelExactMatch));
+    } else {
+      ASSERT_NOK(
+          SanityCheckDBOptions(opts, ConfigOptions::kSanityLevelExactMatch));
+    }
+  }
+
+  Status PersistOptions(const DBOptions& db_opts,
+                        const ColumnFamilyOptions& cf_opts) {
     Status s = fs_->DeleteFile(kOptionsFileName, IOOptions(), nullptr);
     if (!s.ok()) {
       return s;
     }
-    return PersistRocksDBOptions(DBOptions(), {"default"}, {cf_opts},
+    return PersistRocksDBOptions(db_opts, {"default"}, {cf_opts},
                                  kOptionsFileName, fs_.get());
   }
 
+  Status PersistCFOptions(const ColumnFamilyOptions& cf_opts) {
+    return PersistOptions(DBOptions(), cf_opts);
+  }
+
+  Status PersistDBOptions(const DBOptions& db_opts) {
+    return PersistOptions(db_opts, ColumnFamilyOptions());
+  }
+
   const std::string kOptionsFileName = "OPTIONS";
 };
 
-TEST_F(OptionsSanityCheckTest, SanityCheck) {
+TEST_P(OptionsSanityCheckTest, CFOptionsSanityCheck) {
   ColumnFamilyOptions opts;
   Random rnd(301);
 
@@ -3792,11 +3841,7 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) {
     opts.prefix_extractor.reset(NewFixedPrefixTransform(15));
     // expect pass only in
     // ConfigOptions::kSanityLevelLooselyCompatible
-    ASSERT_NOK(
+    SanityCheckCFOptions(opts, false);
-        SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch));
-    ASSERT_OK(SanityCheckCFOptions(
-        opts, ConfigOptions::kSanityLevelLooselyCompatible));
-    ASSERT_OK(SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelNone));
 
     // Change prefix extractor from non-nullptr to nullptr
     opts.prefix_extractor.reset();
@@ -3836,8 +3881,7 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) {
 
     // persist the change
     ASSERT_OK(PersistCFOptions(opts));
-    ASSERT_OK(
+    SanityCheckCFOptions(opts, config_options_.ignore_unsupported_options);
-        SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch));
 
     for (int test = 0; test < 5; ++test) {
       // change the merge operator
@@ -3848,8 +3892,7 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) {
 
       // persist the change
       ASSERT_OK(PersistCFOptions(opts));
-      ASSERT_OK(
+      SanityCheckCFOptions(opts, config_options_.ignore_unsupported_options);
-          SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch));
     }
 
     // Test when going from merge operator -> nullptr
@@ -3860,8 +3903,7 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) {
 
     // persist the change
     ASSERT_OK(PersistCFOptions(opts));
-    ASSERT_OK(
+    SanityCheckCFOptions(opts, true);
-        SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch));
   }
 
   // compaction_filter
@@ -3869,15 +3911,11 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) {
     for (int test = 0; test < 5; ++test) {
       // change the compaction filter
       opts.compaction_filter = test::RandomCompactionFilter(&rnd);
-      ASSERT_NOK(
+      SanityCheckCFOptions(opts, false);
-          SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch));
-      ASSERT_OK(SanityCheckCFOptions(
-          opts, ConfigOptions::kSanityLevelLooselyCompatible));
 
       // persist the change
       ASSERT_OK(PersistCFOptions(opts));
-      ASSERT_OK(
+      SanityCheckCFOptions(opts, config_options_.ignore_unsupported_options);
-          SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch));
       delete opts.compaction_filter;
       opts.compaction_filter = nullptr;
     }
@@ -3889,17 +3927,55 @@ TEST_F(OptionsSanityCheckTest, SanityCheck) {
       // change the compaction filter factory
       opts.compaction_filter_factory.reset(
           test::RandomCompactionFilterFactory(&rnd));
-      ASSERT_NOK(
+      SanityCheckCFOptions(opts, false);
-          SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch));
-      ASSERT_OK(SanityCheckCFOptions(
-          opts, ConfigOptions::kSanityLevelLooselyCompatible));
 
       // persist the change
       ASSERT_OK(PersistCFOptions(opts));
+      SanityCheckCFOptions(opts, config_options_.ignore_unsupported_options);
+    }
+  }
+}
+
+TEST_P(OptionsSanityCheckTest, DBOptionsSanityCheck) {
+  DBOptions opts;
+  Random rnd(301);
+
+  // default DBOptions
+  {
+    ASSERT_OK(PersistDBOptions(opts));
     ASSERT_OK(
-          SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelExactMatch));
+        SanityCheckDBOptions(opts, ConfigOptions::kSanityLevelExactMatch));
   }
+
+  // File checksum generator
+  {
+    class MockFileChecksumGenFactory : public FileChecksumGenFactory {
+     public:
+      static const char* kClassName() { return "Mock"; }
+      const char* Name() const override { return kClassName(); }
+      std::unique_ptr<FileChecksumGenerator> CreateFileChecksumGenerator(
+          const FileChecksumGenContext& /*context*/) override {
+        return nullptr;
       }
+    };
+
+    // Okay to change file_checksum_gen_factory form nullptr to non-nullptr
+    ASSERT_EQ(opts.file_checksum_gen_factory.get(), nullptr);
+    opts.file_checksum_gen_factory.reset(new MockFileChecksumGenFactory());
+
+    // persist the change
+    ASSERT_OK(PersistDBOptions(opts));
+    SanityCheckDBOptions(opts, config_options_.ignore_unsupported_options);
+
+    // Change file_checksum_gen_factory from non-nullptr to nullptr
+    opts.file_checksum_gen_factory.reset();
+    // expect pass as it's safe to change file_checksum_gen_factory
+    // from non-null to null
+    SanityCheckDBOptions(opts, false);
+  }
+  // persist the change
+  ASSERT_OK(PersistDBOptions(opts));
+  ASSERT_OK(SanityCheckDBOptions(opts, ConfigOptions::kSanityLevelExactMatch));
 }
 
 namespace {
@@ -4612,7 +4688,11 @@ TEST_F(ConfigOptionsTest, MergeOperatorFromString) {
   ASSERT_NE(delimiter, nullptr);
   ASSERT_EQ(*delimiter, "&&");
 }
+
+INSTANTIATE_TEST_CASE_P(OptionsSanityCheckTest, OptionsSanityCheckTest,
+                        ::testing::Bool());
 #endif  // !ROCKSDB_LITE
+
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {

util/slice.cc

@@ -218,12 +218,14 @@ Status SliceTransform::CreateFromString(
                                               value, &id, &opt_map);
   if (!status.ok()) {  // GetOptionsMap failed
     return status;
-  }
+  } else if (id.empty() && opt_map.empty()) {
+    result->reset();
+  } else {
 #ifndef ROCKSDB_LITE
     status = config_options.registry->NewSharedObject(id, result);
 #else
-  auto Matches = [](const std::string& input, size_t size, const char* pattern,
+    auto Matches = [](const std::string& input, size_t size,
-                    char sep) {
+                      const char* pattern, char sep) {
       auto plen = strlen(pattern);
       return (size > plen + 2 && input[plen] == sep &&
               StartsWith(input, pattern));
@@ -252,15 +254,13 @@ Status SliceTransform::CreateFromString(
       status = Status::NotSupported("Cannot load object in LITE mode ", id);
     }
 #endif  // ROCKSDB_LITE
-  if (!status.ok()) {
     if (config_options.ignore_unsupported_options && status.IsNotSupported()) {
       return Status::OK();
-    } else {
+    } else if (status.ok()) {
-      return status;
-    }
-  } else if (result->get() != nullptr) {
       SliceTransform* transform = const_cast<SliceTransform*>(result->get());
-    status = transform->ConfigureFromMap(config_options, opt_map);
+      status =
+          Customizable::ConfigureNewObject(config_options, transform, opt_map);
+    }
   }
   return status;
 }  // namespace ROCKSDB_NAMESPACE