From 4bc9df945992f5e8d7c8e8f4ead9c36db110f063 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Fri, 19 Feb 2021 10:25:39 -0800 Subject: [PATCH] Fix handling of Mutable options; Allow DB::SetOptions to update mutable TableFactory Options (#7936) Summary: Added a "only_mutable_options" flag to the ConfigOptions. When set, the Configurable methods will only look at/update options that are marked as kMutable. Fixed DB::SetOptions to allow for the update of any mutable TableFactory options. Fixes https://github.com/facebook/rocksdb/issues/7385. Added tests for the new flag. Updated HISTORY.md Pull Request resolved: https://github.com/facebook/rocksdb/pull/7936 Reviewed By: akankshamahajan15 Differential Revision: D26389646 Pulled By: mrambacher fbshipit-source-id: 6dc247f6e999fa2814059ebbd0af8face109fea0 --- HISTORY.md | 1 + db/column_family.cc | 19 +-- db/db_options_test.cc | 77 ++++++++++ include/rocksdb/convenience.h | 7 + include/rocksdb/utilities/options_type.h | 7 + options/cf_options.cc | 3 +- options/configurable.cc | 184 ++++++++++++++++++----- options/configurable_helper.h | 6 +- options/configurable_test.cc | 88 ++++++++++- options/customizable_helper.h | 15 +- options/customizable_test.cc | 95 +++++++++++- options/db_options.cc | 3 +- options/options_test.cc | 162 ++++++++++++++++++++ 13 files changed, 596 insertions(+), 71 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 1730585d8..3af816a18 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -18,6 +18,7 @@ * In DB::OpenForReadOnly, if any error happens while checking Manifest file path, it was overridden by Status::NotFound. It has been fixed and now actual error is returned. ### Public API Change +* Added a "only_mutable_options" flag to the ConfigOptions. When this flag is "true", the Configurable functions and convenience methods (such as GetDBOptionsFromString) will only deal with options that are marked as mutable. When this flag is true, only options marked as mutable can be configured (a Status::InvalidArgument will be returned) and options not marked as mutable will not be returned or compared. The default is "false", meaning to compare all options. * Add new Append and PositionedAppend APIs to FileSystem to bring the data verification information (data checksum information) from upper layer (e.g., WritableFileWriter) to the storage layer. In this way, the customized FileSystem is able to verify the correctness of data being written to the storage on time. Add checksum_handoff_file_types to DBOptions. User can use this option to control which file types (Currently supported file tyes: kWALFile, kTableFile, kDescriptorFile.) should use the new Append and PositionedAppend APIs to handoff the verification information. Currently, RocksDB only use crc32c to calculate the checksum for write handoff. ## 6.17.0 (01/15/2021) diff --git a/db/column_family.cc b/db/column_family.cc index e0b70687b..dd9ad5600 100644 --- a/db/column_family.cc +++ b/db/column_family.cc @@ -33,6 +33,7 @@ #include "monitoring/thread_status_util.h" #include "options/options_helper.h" #include "port/port.h" +#include "rocksdb/convenience.h" #include "rocksdb/table.h" #include "table/merging_iterator.h" #include "util/autovector.h" @@ -1355,19 +1356,19 @@ Status ColumnFamilyData::ValidateOptions( #ifndef ROCKSDB_LITE Status ColumnFamilyData::SetOptions( - const DBOptions& db_options, + const DBOptions& db_opts, const std::unordered_map& options_map) { - MutableCFOptions new_mutable_cf_options; - Status s = - GetMutableOptionsFromStrings(mutable_cf_options_, options_map, - ioptions_.info_log, &new_mutable_cf_options); + ColumnFamilyOptions cf_opts = + BuildColumnFamilyOptions(initial_cf_options_, mutable_cf_options_); + ConfigOptions config_opts; + config_opts.mutable_options_only = true; + Status s = GetColumnFamilyOptionsFromMap(config_opts, cf_opts, options_map, + &cf_opts); if (s.ok()) { - ColumnFamilyOptions cf_options = - BuildColumnFamilyOptions(initial_cf_options_, new_mutable_cf_options); - s = ValidateOptions(db_options, cf_options); + s = ValidateOptions(db_opts, cf_opts); } if (s.ok()) { - mutable_cf_options_ = new_mutable_cf_options; + mutable_cf_options_ = MutableCFOptions(cf_opts); mutable_cf_options_.RefreshDerivedOptions(ioptions_); } return s; diff --git a/db/db_options_test.cc b/db/db_options_test.cc index 9036735c6..0858e1421 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -129,6 +129,83 @@ TEST_F(DBOptionsTest, GetLatestCFOptions) { GetMutableCFOptionsMap(dbfull()->GetOptions(handles_[1]))); } +TEST_F(DBOptionsTest, SetMutableTableOptions) { + Options options; + options.create_if_missing = true; + options.env = env_; + options.blob_file_size = 16384; + BlockBasedTableOptions bbto; + bbto.no_block_cache = true; + bbto.block_size = 8192; + bbto.block_restart_interval = 7; + + options.table_factory.reset(NewBlockBasedTableFactory(bbto)); + Reopen(options); + + ColumnFamilyHandle* cfh = dbfull()->DefaultColumnFamily(); + Options c_opts = dbfull()->GetOptions(cfh); + const auto* c_bbto = + c_opts.table_factory->GetOptions(); + ASSERT_NE(c_bbto, nullptr); + ASSERT_EQ(c_opts.blob_file_size, 16384); + ASSERT_EQ(c_bbto->no_block_cache, true); + ASSERT_EQ(c_bbto->block_size, 8192); + ASSERT_EQ(c_bbto->block_restart_interval, 7); + ASSERT_OK(dbfull()->SetOptions( + cfh, {{"table_factory.block_size", "16384"}, + {"table_factory.block_restart_interval", "11"}})); + ASSERT_EQ(c_bbto->block_size, 16384); + ASSERT_EQ(c_bbto->block_restart_interval, 11); + + // Now set an option that is not mutable - options should not change + ASSERT_NOK( + dbfull()->SetOptions(cfh, {{"table_factory.no_block_cache", "false"}})); + ASSERT_EQ(c_bbto->no_block_cache, true); + ASSERT_EQ(c_bbto->block_size, 16384); + ASSERT_EQ(c_bbto->block_restart_interval, 11); + + // Set some that are mutable and some that are not - options should not change + ASSERT_NOK(dbfull()->SetOptions( + cfh, {{"table_factory.no_block_cache", "false"}, + {"table_factory.block_size", "8192"}, + {"table_factory.block_restart_interval", "7"}})); + ASSERT_EQ(c_bbto->no_block_cache, true); + ASSERT_EQ(c_bbto->block_size, 16384); + ASSERT_EQ(c_bbto->block_restart_interval, 11); + + // Set some that are mutable and some that do not exist - options should not + // change + ASSERT_NOK(dbfull()->SetOptions( + cfh, {{"table_factory.block_size", "8192"}, + {"table_factory.does_not_exist", "true"}, + {"table_factory.block_restart_interval", "7"}})); + ASSERT_EQ(c_bbto->no_block_cache, true); + ASSERT_EQ(c_bbto->block_size, 16384); + ASSERT_EQ(c_bbto->block_restart_interval, 11); + + // Trying to change the table factory fails + ASSERT_NOK(dbfull()->SetOptions( + cfh, {{"table_factory", TableFactory::kPlainTableName()}})); + + // Set some on the table and some on the Column Family + ASSERT_OK(dbfull()->SetOptions( + cfh, {{"table_factory.block_size", "16384"}, + {"blob_file_size", "32768"}, + {"table_factory.block_restart_interval", "13"}})); + c_opts = dbfull()->GetOptions(cfh); + ASSERT_EQ(c_opts.blob_file_size, 32768); + ASSERT_EQ(c_bbto->block_size, 16384); + ASSERT_EQ(c_bbto->block_restart_interval, 13); + // Set some on the table and a bad one on the ColumnFamily - options should + // not change + ASSERT_NOK(dbfull()->SetOptions( + cfh, {{"table_factory.block_size", "1024"}, + {"no_such_option", "32768"}, + {"table_factory.block_restart_interval", "7"}})); + ASSERT_EQ(c_bbto->block_size, 16384); + ASSERT_EQ(c_bbto->block_restart_interval, 13); +} + TEST_F(DBOptionsTest, SetBytesPerSync) { const size_t kValueSize = 1024 * 1024; // 1MB Options options; diff --git a/include/rocksdb/convenience.h b/include/rocksdb/convenience.h index f861b2fcf..3b4b4240c 100644 --- a/include/rocksdb/convenience.h +++ b/include/rocksdb/convenience.h @@ -56,6 +56,13 @@ struct ConfigOptions { // Whether or not to invoke PrepareOptions after configure is called. bool invoke_prepare_options = true; + // Options can be marked as Mutable (OptionTypeInfo::IsMutable()) or not. + // When "mutable_options_only=false", all options are evaluated. + // When "mutable_options_only="true", any option not marked as Mutable is + // either ignored (in the case of string/equals methods) or results in an + // error (in the case of Configure). + bool mutable_options_only = false; + // The separator between options when converting to a string std::string delimiter = ";"; diff --git a/include/rocksdb/utilities/options_type.h b/include/rocksdb/utilities/options_type.h index 36e1e09f9..d5a192daa 100644 --- a/include/rocksdb/utilities/options_type.h +++ b/include/rocksdb/utilities/options_type.h @@ -507,6 +507,13 @@ class OptionTypeInfo { bool IsEnabled(OptionTypeFlags otf) const { return (flags_ & otf) == otf; } + bool IsEditable(const ConfigOptions& opts) const { + if (opts.mutable_options_only) { + return IsMutable(); + } else { + return true; + } + } bool IsMutable() const { return IsEnabled(OptionTypeFlags::kMutable); } bool IsDeprecated() const { diff --git a/options/cf_options.cc b/options/cf_options.cc index 7e680fd88..2d3c0e247 100644 --- a/options/cf_options.cc +++ b/options/cf_options.cc @@ -712,8 +712,7 @@ class ConfigurableCFOptions : public ConfigurableMutableCFOptions { const ConfigOptions& config_options, const std::unordered_map& opts_map, std::unordered_map* unused) override { - Status s = ConfigurableHelper::ConfigureOptions(config_options, *this, - opts_map, unused); + Status s = Configurable::ConfigureOptions(config_options, opts_map, unused); if (s.ok()) { cf_options_ = BuildColumnFamilyOptions(immutable_, mutable_); s = PrepareOptions(config_options); diff --git a/options/configurable.cc b/options/configurable.cc index 3167c5d06..aa2b957c4 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -161,7 +161,10 @@ Status Configurable::ConfigureOptions( #ifndef ROCKSDB_LITE if (!config_options.ignore_unknown_options) { // If we are not ignoring unused, get the defaults in case we need to reset - GetOptionString(config_options, &curr_opts).PermitUncheckedError(); + ConfigOptions copy = config_options; + copy.depth = ConfigOptions::kDepthDetailed; + copy.delimiter = "; "; + GetOptionString(copy, &curr_opts).PermitUncheckedError(); } #endif // ROCKSDB_LITE Status s = ConfigurableHelper::ConfigureOptions(config_options, *this, @@ -223,9 +226,8 @@ Status Configurable::ConfigureFromString(const ConfigOptions& config_options, Status Configurable::ConfigureOption(const ConfigOptions& config_options, const std::string& name, const std::string& value) { - const std::string& opt_name = GetOptionName(name); - return ConfigurableHelper::ConfigureSingleOption(config_options, *this, - opt_name, value); + return ConfigurableHelper::ConfigureSingleOption(config_options, *this, name, + value); } /** @@ -239,9 +241,16 @@ Status Configurable::ParseOption(const ConfigOptions& config_options, const OptionTypeInfo& opt_info, const std::string& opt_name, const std::string& opt_value, void* opt_ptr) { - if (opt_info.IsMutable() || opt_info.IsConfigurable()) { - return opt_info.Parse(config_options, opt_name, opt_value, opt_ptr); - } else if (prepared_) { + if (opt_info.IsMutable()) { + if (config_options.mutable_options_only) { + // This option is mutable. Treat all of its children as mutable as well + ConfigOptions copy = config_options; + copy.mutable_options_only = false; + return opt_info.Parse(copy, opt_name, opt_value, opt_ptr); + } else { + return opt_info.Parse(config_options, opt_name, opt_value, opt_ptr); + } + } else if (config_options.mutable_options_only) { return Status::InvalidArgument("Option not changeable: " + opt_name); } else { return opt_info.Parse(config_options, opt_name, opt_value, opt_ptr); @@ -364,42 +373,104 @@ Status ConfigurableHelper::ConfigureSomeOptions( Status ConfigurableHelper::ConfigureSingleOption( const ConfigOptions& config_options, Configurable& configurable, const std::string& name, const std::string& value) { - std::string opt_name; + const std::string& opt_name = configurable.GetOptionName(name); + std::string elem_name; void* opt_ptr = nullptr; const auto opt_info = - FindOption(configurable.options_, name, &opt_name, &opt_ptr); + FindOption(configurable.options_, opt_name, &elem_name, &opt_ptr); if (opt_info == nullptr) { return Status::NotFound("Could not find option: ", name); } else { - return ConfigureOption(config_options, configurable, *opt_info, name, - opt_name, value, opt_ptr); + return ConfigureOption(config_options, configurable, *opt_info, opt_name, + elem_name, value, opt_ptr); } } - -Status ConfigurableHelper::ConfigureOption( +Status ConfigurableHelper::ConfigureCustomizableOption( const ConfigOptions& config_options, Configurable& configurable, const OptionTypeInfo& opt_info, const std::string& opt_name, const std::string& name, const std::string& value, void* opt_ptr) { - if (opt_name == name) { - return configurable.ParseOption(config_options, opt_info, opt_name, value, - opt_ptr); - } else if (opt_info.IsCustomizable() && - EndsWith(opt_name, ConfigurableHelper::kIdPropSuffix)) { - return configurable.ParseOption(config_options, opt_info, name, value, - opt_ptr); + Customizable* custom = opt_info.AsRawPointer(opt_ptr); + ConfigOptions copy = config_options; + if (opt_info.IsMutable()) { + // This option is mutable. Pass that property on to any subsequent calls + copy.mutable_options_only = false; + } - } else if (opt_info.IsCustomizable()) { - Customizable* custom = opt_info.AsRawPointer(opt_ptr); - if (value.empty()) { + if (opt_info.IsMutable() || !config_options.mutable_options_only) { + // Either the option is mutable, or we are processing all of the options + if (opt_name == name || + EndsWith(opt_name, ConfigurableHelper::kIdPropSuffix) || + name == ConfigurableHelper::kIdPropName) { + return configurable.ParseOption(copy, opt_info, opt_name, value, opt_ptr); + } else if (value.empty()) { return Status::OK(); } else if (custom == nullptr || !StartsWith(name, custom->GetId() + ".")) { - return configurable.ParseOption(config_options, opt_info, name, value, - opt_ptr); + return configurable.ParseOption(copy, opt_info, name, value, opt_ptr); } else if (value.find("=") != std::string::npos) { - return custom->ConfigureFromString(config_options, value); + return custom->ConfigureFromString(copy, value); + } else { + return custom->ConfigureOption(copy, name, value); + } + } else { + // We are processing immutable options, which means that we cannot change + // the Customizable object itself, but could change its mutable properties. + // Check to make sure that nothing is trying to change the Customizable + if (custom == nullptr) { + // We do not have a Customizable to configure. This is OK if the + // value is empty (nothing being configured) but an error otherwise + if (value.empty()) { + return Status::OK(); + } else { + return Status::InvalidArgument("Option not changeable: " + opt_name); + } + } else if (EndsWith(opt_name, ConfigurableHelper::kIdPropSuffix) || + name == ConfigurableHelper::kIdPropName) { + // We have a property of the form "id=value" or "table.id=value" + // This is OK if we ID/value matches the current customizable object + if (custom->GetId() == value) { + return Status::OK(); + } else { + return Status::InvalidArgument("Option not changeable: " + opt_name); + } + } else if (opt_name == name) { + // The properties are of one of forms: + // name = { id = id; prop1 = value1; ... } + // name = { prop1=value1; prop2=value2; ... } + // name = ID + // Convert the value to a map and extract the ID + // If the ID does not match that of the current customizable, return an + // error. Otherwise, update the current customizable via the properties + // map + std::unordered_map props; + std::string id; + Status s = GetOptionsMap(value, custom->GetId(), &id, &props); + if (!s.ok()) { + return s; + } else if (custom->GetId() != id) { + return Status::InvalidArgument("Option not changeable: " + opt_name); + } else if (props.empty()) { + return Status::OK(); + } else { + return custom->ConfigureFromMap(copy, props); + } } else { - return custom->ConfigureOption(config_options, name, value); + // Attempting to configure one of the properties of the customizable + // Let it through + return custom->ConfigureOption(copy, name, value); } + } +} + +Status ConfigurableHelper::ConfigureOption( + const ConfigOptions& config_options, Configurable& configurable, + const OptionTypeInfo& opt_info, const std::string& opt_name, + const std::string& name, const std::string& value, void* opt_ptr) { + if (opt_info.IsCustomizable()) { + return ConfigureCustomizableOption(config_options, configurable, opt_info, + opt_name, name, value, opt_ptr); + } else if (opt_name == name) { + return configurable.ParseOption(config_options, opt_info, opt_name, value, + opt_ptr); } else if (opt_info.IsStruct() || opt_info.IsConfigurable()) { return configurable.ParseOption(config_options, opt_info, name, value, opt_ptr); @@ -521,8 +592,25 @@ Status ConfigurableHelper::SerializeOptions(const ConfigOptions& config_options, const auto& opt_info = map_iter.second; if (opt_info.ShouldSerialize()) { std::string value; - Status s = opt_info.Serialize(config_options, prefix + opt_name, - opt_iter.opt_ptr, &value); + Status s; + if (!config_options.mutable_options_only) { + s = opt_info.Serialize(config_options, prefix + opt_name, + opt_iter.opt_ptr, &value); + } else if (opt_info.IsMutable()) { + ConfigOptions copy = config_options; + copy.mutable_options_only = false; + s = opt_info.Serialize(copy, prefix + opt_name, opt_iter.opt_ptr, + &value); + } else if (opt_info.IsConfigurable()) { + // If it is a Configurable and we are either printing all of the + // details or not printing only the name, this option should be + // included in the list + if (config_options.IsDetailed() || + !opt_info.IsEnabled(OptionTypeFlags::kStringNameOnly)) { + s = opt_info.Serialize(config_options, prefix + opt_name, + opt_iter.opt_ptr, &value); + } + } if (!s.ok()) { return s; } else if (!value.empty()) { @@ -551,7 +639,7 @@ Status Configurable::GetOptionNames( } Status ConfigurableHelper::ListOptions( - const ConfigOptions& /*config_options*/, const Configurable& configurable, + const ConfigOptions& config_options, const Configurable& configurable, const std::string& prefix, std::unordered_set* result) { Status status; for (auto const& opt_iter : configurable.options_) { @@ -561,7 +649,11 @@ Status ConfigurableHelper::ListOptions( // If the option is no longer used in rocksdb and marked as deprecated, // we skip it in the serialization. if (!opt_info.IsDeprecated() && !opt_info.IsAlias()) { - result->emplace(prefix + opt_name); + if (!config_options.mutable_options_only) { + result->emplace(prefix + opt_name); + } else if (opt_info.IsMutable()) { + result->emplace(prefix + opt_name); + } } } } @@ -626,11 +718,23 @@ bool ConfigurableHelper::AreEquivalent(const ConfigOptions& config_options, return false; } else { for (const auto& map_iter : *(o.type_map)) { - if (config_options.IsCheckEnabled(map_iter.second.GetSanityLevel()) && - !this_one.OptionsAreEqual(config_options, map_iter.second, - map_iter.first, this_offset, - that_offset, mismatch)) { - return false; + const auto& opt_info = map_iter.second; + if (config_options.IsCheckEnabled(opt_info.GetSanityLevel())) { + if (!config_options.mutable_options_only) { + if (!this_one.OptionsAreEqual(config_options, opt_info, + map_iter.first, this_offset, + that_offset, mismatch)) { + return false; + } + } else if (opt_info.IsMutable()) { + ConfigOptions copy = config_options; + copy.mutable_options_only = false; + if (!this_one.OptionsAreEqual(copy, opt_info, map_iter.first, + this_offset, that_offset, + mismatch)) { + return false; + } + } } } } @@ -641,9 +745,13 @@ bool ConfigurableHelper::AreEquivalent(const ConfigOptions& config_options, #endif // ROCKSDB_LITE Status ConfigurableHelper::GetOptionsMap( - const std::string& value, std::string* id, + const std::string& value, const Customizable* customizable, std::string* id, std::unordered_map* props) { - return GetOptionsMap(value, "", id, props); + if (customizable != nullptr) { + return GetOptionsMap(value, customizable->GetId(), id, props); + } else { + return GetOptionsMap(value, "", id, props); + } } Status ConfigurableHelper::GetOptionsMap( diff --git a/options/configurable_helper.h b/options/configurable_helper.h index adc070548..bc54a099f 100644 --- a/options/configurable_helper.h +++ b/options/configurable_helper.h @@ -108,7 +108,7 @@ class ConfigurableHelper { // @return InvalidArgument if the value could not be converted to a map or // there was or there is no id property in the map. static Status GetOptionsMap( - const std::string& opt_value, std::string* id, + const std::string& opt_value, const Customizable* custom, std::string* id, std::unordered_map* options); static Status GetOptionsMap( const std::string& opt_value, const std::string& default_id, @@ -245,6 +245,10 @@ class ConfigurableHelper { const std::vector& options, const std::string& name, std::string* opt_name, void** opt_ptr); + static Status ConfigureCustomizableOption( + const ConfigOptions& config_options, Configurable& configurable, + const OptionTypeInfo& opt_info, const std::string& opt_name, + const std::string& name, const std::string& value, void* opt_ptr); #endif // ROCKSDB_LITE }; diff --git a/options/configurable_test.cc b/options/configurable_test.cc index fe0c76561..68fcf963f 100644 --- a/options/configurable_test.cc +++ b/options/configurable_test.cc @@ -45,6 +45,22 @@ class StringLogger : public Logger { private: std::string string_; }; +static std::unordered_map struct_option_info = { +#ifndef ROCKSDB_LITE + {"struct", OptionTypeInfo::Struct("struct", &simple_option_info, 0, + OptionVerificationType::kNormal, + OptionTypeFlags::kMutable)}, +#endif // ROCKSDB_LITE +}; + +static std::unordered_map imm_struct_option_info = + { +#ifndef ROCKSDB_LITE + {"struct", OptionTypeInfo::Struct("struct", &simple_option_info, 0, + OptionVerificationType::kNormal, + OptionTypeFlags::kNone)}, +#endif // ROCKSDB_LITE +}; class SimpleConfigurable : public TestConfigurable { public: @@ -322,6 +338,71 @@ TEST_F(ConfigurableTest, PrepareOptionsTest) { ASSERT_EQ(*up, 0); } +TEST_F(ConfigurableTest, MutableOptionsTest) { + static std::unordered_map imm_option_info = { +#ifndef ROCKSDB_LITE + {"imm", OptionTypeInfo::Struct("imm", &simple_option_info, 0, + OptionVerificationType::kNormal, + OptionTypeFlags::kNone)}, +#endif // ROCKSDB_LITE + }; + + class MutableConfigurable : public SimpleConfigurable { + public: + MutableConfigurable() + : SimpleConfigurable("mutable", TestConfigMode::kDefaultMode | + TestConfigMode::kUniqueMode | + TestConfigMode::kSharedMode) { + ConfigurableHelper::RegisterOptions(*this, "struct", &options_, + &struct_option_info); + ConfigurableHelper::RegisterOptions(*this, "imm", &options_, + &imm_option_info); + } + }; + MutableConfigurable mc; + ConfigOptions options = config_options_; + + ASSERT_OK(mc.ConfigureOption(options, "bool", "true")); + ASSERT_OK(mc.ConfigureOption(options, "int", "42")); + auto* opts = mc.GetOptions("mutable"); + ASSERT_NE(opts, nullptr); + ASSERT_EQ(opts->i, 42); + ASSERT_EQ(opts->b, true); + ASSERT_OK(mc.ConfigureOption(options, "struct", "{bool=false;}")); + ASSERT_OK(mc.ConfigureOption(options, "imm", "{int=55;}")); + + options.mutable_options_only = true; + + // Now only mutable options should be settable. + ASSERT_NOK(mc.ConfigureOption(options, "bool", "true")); + ASSERT_OK(mc.ConfigureOption(options, "int", "24")); + ASSERT_EQ(opts->i, 24); + ASSERT_EQ(opts->b, false); + ASSERT_NOK(mc.ConfigureFromString(options, "bool=false;int=33;")); + ASSERT_EQ(opts->i, 24); + ASSERT_EQ(opts->b, false); + + // Setting options through an immutable struct fails + ASSERT_NOK(mc.ConfigureOption(options, "imm", "{int=55;}")); + ASSERT_NOK(mc.ConfigureOption(options, "imm.int", "55")); + ASSERT_EQ(opts->i, 24); + ASSERT_EQ(opts->b, false); + + // Setting options through an mutable struct succeeds + ASSERT_OK(mc.ConfigureOption(options, "struct", "{int=44;}")); + ASSERT_EQ(opts->i, 44); + ASSERT_OK(mc.ConfigureOption(options, "struct.int", "55")); + ASSERT_EQ(opts->i, 55); + + // Setting nested immutable configurable options fail + ASSERT_NOK(mc.ConfigureOption(options, "shared", "{bool=true;}")); + ASSERT_NOK(mc.ConfigureOption(options, "shared.bool", "true")); + + // Setting nested mutable configurable options succeeds + ASSERT_OK(mc.ConfigureOption(options, "unique", "{bool=true}")); + ASSERT_OK(mc.ConfigureOption(options, "unique.bool", "true")); +} + TEST_F(ConfigurableTest, DeprecatedOptionsTest) { static std::unordered_map deprecated_option_info = { @@ -453,13 +534,6 @@ TEST_F(ConfigurableTest, MatchesTest) { } static Configurable* SimpleStructFactory() { - static std::unordered_map struct_option_info = { -#ifndef ROCKSDB_LITE - {"struct", OptionTypeInfo::Struct("struct", &simple_option_info, 0, - OptionVerificationType::kNormal, - OptionTypeFlags::kMutable)}, -#endif // ROCKSDB_LITE - }; return SimpleConfigurable::Create( "simple-struct", TestConfigMode::kDefaultMode, &struct_option_info); } diff --git a/options/customizable_helper.h b/options/customizable_helper.h index c9f6f747e..ff07e5498 100644 --- a/options/customizable_helper.h +++ b/options/customizable_helper.h @@ -61,7 +61,8 @@ static Status LoadSharedObject(const ConfigOptions& config_options, std::shared_ptr* result) { std::string id; std::unordered_map opt_map; - Status status = ConfigurableHelper::GetOptionsMap(value, &id, &opt_map); + Status status = + ConfigurableHelper::GetOptionsMap(value, result->get(), &id, &opt_map); if (!status.ok()) { // GetOptionsMap failed return status; } @@ -75,7 +76,7 @@ static Status LoadSharedObject(const ConfigOptions& config_options, } #endif if (func == nullptr || !func(id, result)) { // No factory, or it failed - if (id.empty() && opt_map.empty()) { + if (value.empty()) { // No Id and no options. Clear the object result->reset(); return Status::OK(); @@ -119,7 +120,8 @@ static Status LoadUniqueObject(const ConfigOptions& config_options, std::unique_ptr* result) { std::string id; std::unordered_map opt_map; - Status status = ConfigurableHelper::GetOptionsMap(value, &id, &opt_map); + Status status = + ConfigurableHelper::GetOptionsMap(value, result->get(), &id, &opt_map); if (!status.ok()) { // GetOptionsMap failed return status; } @@ -133,7 +135,7 @@ static Status LoadUniqueObject(const ConfigOptions& config_options, } #endif if (func == nullptr || !func(id, result)) { // No factory, or it failed - if (id.empty() && opt_map.empty()) { + if (value.empty()) { // No Id and no options. Clear the object result->reset(); return Status::OK(); @@ -175,7 +177,8 @@ static Status LoadStaticObject(const ConfigOptions& config_options, const StaticFactoryFunc& func, T** result) { std::string id; std::unordered_map opt_map; - Status status = ConfigurableHelper::GetOptionsMap(value, &id, &opt_map); + Status status = + ConfigurableHelper::GetOptionsMap(value, *result, &id, &opt_map); if (!status.ok()) { // GetOptionsMap failed return status; } @@ -189,7 +192,7 @@ static Status LoadStaticObject(const ConfigOptions& config_options, } #endif if (func == nullptr || !func(id, result)) { // No factory, or it failed - if (id.empty() && opt_map.empty()) { + if (value.empty()) { // No Id and no options. Clear the object *result = nullptr; return Status::OK(); diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 100ed6787..4726e4866 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -90,7 +90,7 @@ static std::unordered_map a_option_info = { #ifndef ROCKSDB_LITE {"int", {offsetof(struct AOptions, i), OptionType::kInt, - OptionVerificationType::kNormal, OptionTypeFlags::kNone}}, + OptionVerificationType::kNormal, OptionTypeFlags::kMutable}}, {"bool", {offsetof(struct AOptions, b), OptionType::kBoolean, OptionVerificationType::kNormal, OptionTypeFlags::kNone}}, @@ -131,7 +131,7 @@ static std::unordered_map b_option_info = { #ifndef ROCKSDB_LITE {"string", {offsetof(struct BOptions, s), OptionType::kString, - OptionVerificationType::kNormal, OptionTypeFlags::kNone}}, + OptionVerificationType::kNormal, OptionTypeFlags::kMutable}}, {"bool", {offsetof(struct BOptions, b), OptionType::kBoolean, OptionVerificationType::kNormal, OptionTypeFlags::kNone}}, @@ -217,6 +217,7 @@ const FactoryFunc& s_func = struct SimpleOptions { bool b = true; + bool is_mutable = true; std::unique_ptr cu; std::shared_ptr cs; TestCustomizable* cp = nullptr; @@ -253,6 +254,16 @@ class SimpleConfigurable : public Configurable { const std::unordered_map* map) { ConfigurableHelper::RegisterOptions(*this, "simple", &simple_, map); } + + bool IsPrepared() const override { + if (simple_.is_mutable) { + return false; + } else { + return Configurable::IsPrepared(); + } + } + + private: }; class CustomizableTest : public testing::Test { @@ -536,7 +547,7 @@ TEST_F(CustomizableTest, NewCustomizableTest) { ASSERT_OK(base->ConfigureFromString(config_options_, "unique={id=A_2;int=1;bool=false}")); ASSERT_EQ(A_count, 3); // Created another A - ASSERT_OK(base->ConfigureFromString(config_options_, "unique=")); + ASSERT_OK(base->ConfigureFromString(config_options_, "unique.id=")); ASSERT_EQ(simple->cu, nullptr); ASSERT_EQ(A_count, 3); } @@ -600,9 +611,9 @@ TEST_F(CustomizableTest, FactoryFunctionTest) { ASSERT_NE(pointer, nullptr); delete pointer; pointer = nullptr; - ASSERT_OK(TestCustomizable::CreateFromString(ignore, "", &shared)); - ASSERT_OK(TestCustomizable::CreateFromString(ignore, "", &unique)); - ASSERT_OK(TestCustomizable::CreateFromString(ignore, "", &pointer)); + ASSERT_OK(TestCustomizable::CreateFromString(ignore, "id=", &shared)); + ASSERT_OK(TestCustomizable::CreateFromString(ignore, "id=", &unique)); + ASSERT_OK(TestCustomizable::CreateFromString(ignore, "id=", &pointer)); ASSERT_EQ(shared.get(), nullptr); ASSERT_EQ(unique.get(), nullptr); ASSERT_EQ(pointer, nullptr); @@ -613,6 +624,78 @@ TEST_F(CustomizableTest, FactoryFunctionTest) { ASSERT_EQ(pointer, nullptr); } +TEST_F(CustomizableTest, MutableOptionsTest) { + static std::unordered_map mutable_option_info = { + {"mutable", + OptionTypeInfo::AsCustomSharedPtr( + 0, OptionVerificationType::kNormal, OptionTypeFlags::kMutable)}}; + static std::unordered_map immutable_option_info = + {{"immutable", + OptionTypeInfo::AsCustomSharedPtr( + 0, OptionVerificationType::kNormal, OptionTypeFlags::kNone)}}; + + class MutableCustomizable : public Customizable { + private: + std::shared_ptr mutable_; + std::shared_ptr immutable_; + + public: + MutableCustomizable() { + ConfigurableHelper::RegisterOptions(*this, "mutable", &mutable_, + &mutable_option_info); + ConfigurableHelper::RegisterOptions(*this, "immutable", &immutable_, + &immutable_option_info); + } + const char* Name() const override { return "MutableCustomizable"; } + }; + MutableCustomizable mc; + + ConfigOptions options = config_options_; + ASSERT_FALSE(mc.IsPrepared()); + ASSERT_OK(mc.ConfigureOption(options, "mutable", "{id=B;}")); + ASSERT_OK(mc.ConfigureOption(options, "immutable", "{id=A; int=10}")); + auto* mm = mc.GetOptions>("mutable"); + auto* im = mc.GetOptions>("immutable"); + ASSERT_NE(mm, nullptr); + ASSERT_NE(mm->get(), nullptr); + ASSERT_NE(im, nullptr); + ASSERT_NE(im->get(), nullptr); + + // Now only deal with mutable options + options.mutable_options_only = true; + + // Setting nested immutable customizable options fails + ASSERT_NOK(mc.ConfigureOption(options, "immutable", "{id=B;}")); + ASSERT_NOK(mc.ConfigureOption(options, "immutable.id", "B")); + ASSERT_NOK(mc.ConfigureOption(options, "immutable.bool", "true")); + ASSERT_NOK(mc.ConfigureOption(options, "immutable", "bool=true")); + ASSERT_NOK(mc.ConfigureOption(options, "immutable", "{int=11;bool=true}")); + auto* im_a = im->get()->GetOptions("A"); + ASSERT_NE(im_a, nullptr); + ASSERT_EQ(im_a->i, 10); + ASSERT_EQ(im_a->b, false); + + // Setting nested mutable customizable options succeeds but the object did not + // change + ASSERT_OK(mc.ConfigureOption(options, "immutable.int", "11")); + ASSERT_EQ(im_a->i, 11); + ASSERT_EQ(im_a, im->get()->GetOptions("A")); + + // The mutable configurable itself can be changed + ASSERT_OK(mc.ConfigureOption(options, "mutable.id", "A")); + ASSERT_OK(mc.ConfigureOption(options, "mutable", "A")); + ASSERT_OK(mc.ConfigureOption(options, "mutable", "{id=A}")); + ASSERT_OK(mc.ConfigureOption(options, "mutable", "{bool=true}")); + + // The Nested options in the mutable object can be changed + ASSERT_OK(mc.ConfigureOption(options, "mutable", "{bool=true}")); + auto* mm_a = mm->get()->GetOptions("A"); + ASSERT_EQ(mm_a->b, true); + ASSERT_OK(mc.ConfigureOption(options, "mutable", "{int=11;bool=false}")); + mm_a = mm->get()->GetOptions("A"); + ASSERT_EQ(mm_a->i, 11); + ASSERT_EQ(mm_a->b, false); +} #endif // !ROCKSDB_LITE } // namespace ROCKSDB_NAMESPACE diff --git a/options/db_options.cc b/options/db_options.cc index 8e587117b..9eb5f8f0b 100644 --- a/options/db_options.cc +++ b/options/db_options.cc @@ -466,8 +466,7 @@ class DBOptionsConfigurable : public MutableDBConfigurable { const ConfigOptions& config_options, const std::unordered_map& opts_map, std::unordered_map* unused) override { - Status s = ConfigurableHelper::ConfigureOptions(config_options, *this, - opts_map, unused); + Status s = Configurable::ConfigureOptions(config_options, opts_map, unused); if (s.ok()) { db_options_ = BuildDBOptions(immutable_, mutable_); s = PrepareOptions(config_options); diff --git a/options/options_test.cc b/options/options_test.cc index b15be0206..dde90e746 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -1371,6 +1371,7 @@ TEST_F(OptionsTest, MutableTableOptions) { ASSERT_EQ(bbto->block_size, 1024); ASSERT_OK(bbtf->PrepareOptions(config_options)); ASSERT_TRUE(bbtf->IsPrepared()); + config_options.mutable_options_only = true; ASSERT_OK(bbtf->ConfigureOption(config_options, "block_size", "1024")); ASSERT_EQ(bbto->block_align, true); ASSERT_NOK(bbtf->ConfigureOption(config_options, "block_align", "false")); @@ -1390,6 +1391,79 @@ TEST_F(OptionsTest, MutableTableOptions) { ASSERT_EQ(bbto->block_size, 8192); } +TEST_F(OptionsTest, MutableCFOptions) { + ConfigOptions config_options; + ColumnFamilyOptions cf_opts; + + ASSERT_OK(GetColumnFamilyOptionsFromString( + config_options, cf_opts, + "paranoid_file_checks=true; block_based_table_factory.block_align=false; " + "block_based_table_factory.block_size=8192;", + &cf_opts)); + ASSERT_TRUE(cf_opts.paranoid_file_checks); + ASSERT_NE(cf_opts.table_factory.get(), nullptr); + const auto bbto = cf_opts.table_factory->GetOptions(); + ASSERT_NE(bbto, nullptr); + ASSERT_EQ(bbto->block_size, 8192); + ASSERT_EQ(bbto->block_align, false); + std::unordered_map unused_opts; + ASSERT_OK(GetColumnFamilyOptionsFromMap( + config_options, cf_opts, {{"paranoid_file_checks", "false"}}, &cf_opts)); + ASSERT_EQ(cf_opts.paranoid_file_checks, false); + + ASSERT_OK(GetColumnFamilyOptionsFromMap( + config_options, cf_opts, + {{"block_based_table_factory.block_size", "16384"}}, &cf_opts)); + ASSERT_EQ(bbto, cf_opts.table_factory->GetOptions()); + ASSERT_EQ(bbto->block_size, 16384); + + config_options.mutable_options_only = true; + // Force consistency checks is not mutable + ASSERT_NOK(GetColumnFamilyOptionsFromMap( + config_options, cf_opts, {{"force_consistency_checks", "true"}}, + &cf_opts)); + + // Attempt to change the table. It is not mutable, so this should fail and + // leave the original intact + ASSERT_NOK(GetColumnFamilyOptionsFromMap( + config_options, cf_opts, {{"table_factory", "PlainTable"}}, &cf_opts)); + ASSERT_NOK(GetColumnFamilyOptionsFromMap( + config_options, cf_opts, {{"table_factory.id", "PlainTable"}}, &cf_opts)); + ASSERT_NE(cf_opts.table_factory.get(), nullptr); + ASSERT_EQ(bbto, cf_opts.table_factory->GetOptions()); + + // Change the block size. Should update the value in the current table + ASSERT_OK(GetColumnFamilyOptionsFromMap( + config_options, cf_opts, + {{"block_based_table_factory.block_size", "8192"}}, &cf_opts)); + ASSERT_EQ(bbto, cf_opts.table_factory->GetOptions()); + ASSERT_EQ(bbto->block_size, 8192); + + // Attempt to turn off block cache fails, as this option is not mutable + ASSERT_NOK(GetColumnFamilyOptionsFromMap( + config_options, cf_opts, + {{"block_based_table_factory.no_block_cache", "true"}}, &cf_opts)); + ASSERT_EQ(bbto, cf_opts.table_factory->GetOptions()); + + // Attempt to change the block size via a config string/map. Should update + // the current value + ASSERT_OK(GetColumnFamilyOptionsFromMap( + config_options, cf_opts, + {{"block_based_table_factory", "{block_size=32768}"}}, &cf_opts)); + ASSERT_EQ(bbto, cf_opts.table_factory->GetOptions()); + ASSERT_EQ(bbto->block_size, 32768); + + // Attempt to change the block size and no cache through the map. Should + // fail, leaving the old values intact + ASSERT_NOK(GetColumnFamilyOptionsFromMap( + config_options, cf_opts, + {{"block_based_table_factory", + "{block_size=16384; no_block_cache=true}"}}, + &cf_opts)); + ASSERT_EQ(bbto, cf_opts.table_factory->GetOptions()); + ASSERT_EQ(bbto->block_size, 32768); +} + #endif // !ROCKSDB_LITE Status StringToMap( @@ -1582,6 +1656,94 @@ TEST_F(OptionsTest, GetStringFromCompressionType) { ASSERT_NOK( GetStringFromCompressionType(&res, static_cast(-10))); } + +TEST_F(OptionsTest, OnlyMutableDBOptions) { + std::string opt_str; + Random rnd(302); + ConfigOptions cfg_opts; + DBOptions db_opts; + DBOptions mdb_opts; + std::unordered_set m_names; + std::unordered_set a_names; + + test::RandomInitDBOptions(&db_opts, &rnd); + auto db_config = DBOptionsAsConfigurable(db_opts); + + // Get all of the DB Option names (mutable or not) + ASSERT_OK(db_config->GetOptionNames(cfg_opts, &a_names)); + + // Get only the mutable options from db_opts and set those in mdb_opts + cfg_opts.mutable_options_only = true; + + // Get only the Mutable DB Option names + ASSERT_OK(db_config->GetOptionNames(cfg_opts, &m_names)); + ASSERT_OK(GetStringFromDBOptions(cfg_opts, db_opts, &opt_str)); + ASSERT_OK(GetDBOptionsFromString(cfg_opts, mdb_opts, opt_str, &mdb_opts)); + std::string mismatch; + // Comparing only the mutable options, the two are equivalent + auto mdb_config = DBOptionsAsConfigurable(mdb_opts); + ASSERT_TRUE(mdb_config->AreEquivalent(cfg_opts, db_config.get(), &mismatch)); + ASSERT_TRUE(db_config->AreEquivalent(cfg_opts, mdb_config.get(), &mismatch)); + + ASSERT_GT(a_names.size(), m_names.size()); + for (const auto& n : m_names) { + std::string m, d; + ASSERT_OK(mdb_config->GetOption(cfg_opts, n, &m)); + ASSERT_OK(db_config->GetOption(cfg_opts, n, &d)); + ASSERT_EQ(m, d); + } + + cfg_opts.mutable_options_only = false; + // Comparing all of the options, the two are not equivalent + ASSERT_FALSE(mdb_config->AreEquivalent(cfg_opts, db_config.get(), &mismatch)); + ASSERT_FALSE(db_config->AreEquivalent(cfg_opts, mdb_config.get(), &mismatch)); +} + +TEST_F(OptionsTest, OnlyMutableCFOptions) { + std::string opt_str; + Random rnd(302); + ConfigOptions cfg_opts; + DBOptions db_opts; + ColumnFamilyOptions mcf_opts; + ColumnFamilyOptions cf_opts; + std::unordered_set m_names; + std::unordered_set a_names; + + test::RandomInitCFOptions(&cf_opts, db_opts, &rnd); + auto cf_config = CFOptionsAsConfigurable(cf_opts); + + // Get all of the CF Option names (mutable or not) + ASSERT_OK(cf_config->GetOptionNames(cfg_opts, &a_names)); + + // Get only the mutable options from cf_opts and set those in mcf_opts + cfg_opts.mutable_options_only = true; + // Get only the Mutable CF Option names + ASSERT_OK(cf_config->GetOptionNames(cfg_opts, &m_names)); + ASSERT_OK(GetStringFromColumnFamilyOptions(cfg_opts, cf_opts, &opt_str)); + ASSERT_OK( + GetColumnFamilyOptionsFromString(cfg_opts, mcf_opts, opt_str, &mcf_opts)); + std::string mismatch; + + auto mcf_config = CFOptionsAsConfigurable(mcf_opts); + // Comparing only the mutable options, the two are equivalent + ASSERT_TRUE(mcf_config->AreEquivalent(cfg_opts, cf_config.get(), &mismatch)); + ASSERT_TRUE(cf_config->AreEquivalent(cfg_opts, mcf_config.get(), &mismatch)); + + ASSERT_GT(a_names.size(), m_names.size()); + for (const auto& n : m_names) { + std::string m, d; + ASSERT_OK(mcf_config->GetOption(cfg_opts, n, &m)); + ASSERT_OK(cf_config->GetOption(cfg_opts, n, &d)); + ASSERT_EQ(m, d); + } + + cfg_opts.mutable_options_only = false; + // Comparing all of the options, the two are not equivalent + ASSERT_FALSE(mcf_config->AreEquivalent(cfg_opts, cf_config.get(), &mismatch)); + ASSERT_FALSE(cf_config->AreEquivalent(cfg_opts, mcf_config.get(), &mismatch)); + + delete cf_opts.compaction_filter; +} #endif // !ROCKSDB_LITE TEST_F(OptionsTest, ConvertOptionsTest) {