From 6dd7279bc661d3053dc2dc208bac28c25b31f7e4 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Mon, 31 Jul 2023 16:02:54 -0400 Subject: [PATCH 1/7] Add a SerializeOptions method to Configurable Converts an object to its serialized name-value properties. --- db/compaction/compaction_service_job.cc | 20 +++- env/composite_env.cc | 37 +++----- env/composite_env_wrapper.h | 5 +- env/env.cc | 44 ++++----- env/file_system.cc | 26 ++---- include/rocksdb/configurable.h | 5 +- include/rocksdb/convenience.h | 7 ++ include/rocksdb/customizable.h | 7 +- include/rocksdb/env.h | 5 +- include/rocksdb/file_system.h | 5 +- include/rocksdb/system_clock.h | 5 +- include/rocksdb/utilities/options_type.h | 46 ++++----- memory/memory_allocator.cc | 5 +- options/cf_options.cc | 5 +- options/configurable.cc | 76 ++++++--------- options/configurable_helper.h | 7 +- options/customizable.cc | 27 ++---- options/customizable_test.cc | 4 +- options/db_options.cc | 19 ++-- options/options_helper.cc | 113 +++++++++++++++++++++-- options/options_test.cc | 2 +- 21 files changed, 260 insertions(+), 210 deletions(-) diff --git a/db/compaction/compaction_service_job.cc b/db/compaction/compaction_service_job.cc index 3149bb5002..db64085ce0 100644 --- a/db/compaction/compaction_service_job.cc +++ b/db/compaction/compaction_service_job.cc @@ -693,8 +693,8 @@ static std::unordered_map cs_result_type_info = { const auto status_obj = static_cast(addr); StatusSerializationAdapter adapter(*status_obj); std::string result; - Status s = OptionTypeInfo::SerializeType(opts, status_adapter_type_info, - &adapter, &result); + Status s = OptionTypeInfo::TypeToString( + opts, "", status_adapter_type_info, &adapter, &result); *value = "{" + result + "}"; return s; }, @@ -770,7 +770,13 @@ Status CompactionServiceInput::Write(std::string* output) { output->append(buf, sizeof(BinaryFormatVersion)); ConfigOptions cf; cf.invoke_prepare_options = false; - return OptionTypeInfo::SerializeType(cf, cs_input_type_info, this, output); + std::unordered_map options; + Status s = + OptionTypeInfo::SerializeType(cf, cs_input_type_info, this, &options); + if (s.ok()) { + output->append(cf.ToString("", options) + cf.delimiter); + } + return s; } Status CompactionServiceResult::Read(const std::string& data_str, @@ -799,7 +805,13 @@ Status CompactionServiceResult::Write(std::string* output) { output->append(buf, sizeof(BinaryFormatVersion)); ConfigOptions cf; cf.invoke_prepare_options = false; - return OptionTypeInfo::SerializeType(cf, cs_result_type_info, this, output); + std::unordered_map options; + Status s = + OptionTypeInfo::SerializeType(cf, cs_result_type_info, this, &options); + if (s.ok()) { + output->append(cf.ToString("", options) + cf.delimiter); + } + return s; } #ifndef NDEBUG diff --git a/env/composite_env.cc b/env/composite_env.cc index 8ddc9a1a6c..8461c2654b 100644 --- a/env/composite_env.cc +++ b/env/composite_env.cc @@ -391,7 +391,7 @@ Status CompositeEnv::NewDirectory(const std::string& name, namespace { static std::unordered_map env_wrapper_type_info = { - {"target", + {Customizable::kTargetPropName(), OptionTypeInfo(0, OptionType::kUnknown, OptionVerificationType::kByName, OptionTypeFlags::kDontSerialize) .SetParseFunc([](const ConfigOptions& opts, @@ -482,14 +482,13 @@ Status CompositeEnvWrapper::PrepareOptions(const ConfigOptions& options) { return Env::PrepareOptions(options); } -std::string CompositeEnvWrapper::SerializeOptions( - const ConfigOptions& config_options, const std::string& header) const { - auto options = CompositeEnv::SerializeOptions(config_options, header); +Status CompositeEnvWrapper::SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const { if (target_.env != nullptr && target_.env != Env::Default()) { - options.append("target="); - options.append(target_.env->ToString(config_options)); + options->insert({kTargetPropName(), target_.env->ToString(config_options)}); } - return options; + return CompositeEnv::SerializeOptions(config_options, options); } EnvWrapper::EnvWrapper(Env* t) : target_(t) { @@ -511,24 +510,14 @@ Status EnvWrapper::PrepareOptions(const ConfigOptions& options) { return Env::PrepareOptions(options); } -std::string EnvWrapper::SerializeOptions(const ConfigOptions& config_options, - const std::string& header) const { - auto parent = Env::SerializeOptions(config_options, ""); - if (config_options.IsShallow() || target_.env == nullptr || - target_.env == Env::Default()) { - return parent; - } else { - std::string result = header; - if (!StartsWith(parent, OptionTypeInfo::kIdPropName())) { - result.append(OptionTypeInfo::kIdPropName()).append("="); - } - result.append(parent); - if (!EndsWith(result, config_options.delimiter)) { - result.append(config_options.delimiter); - } - result.append("target=").append(target_.env->ToString(config_options)); - return result; +Status EnvWrapper::SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const { + if (!config_options.IsShallow() && target_.env != nullptr && + target_.env != Env::Default()) { + options->insert({kTargetPropName(), target_.env->ToString(config_options)}); } + return Env::SerializeOptions(config_options, options); } } // namespace ROCKSDB_NAMESPACE diff --git a/env/composite_env_wrapper.h b/env/composite_env_wrapper.h index 003c50658b..3629282a2c 100644 --- a/env/composite_env_wrapper.h +++ b/env/composite_env_wrapper.h @@ -289,8 +289,9 @@ class CompositeEnvWrapper : public CompositeEnv { const Customizable* Inner() const override { return target_.env; } Status PrepareOptions(const ConfigOptions& options) override; - std::string SerializeOptions(const ConfigOptions& config_options, - const std::string& header) const override; + Status SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const override; // Return the target to which this Env forwards all calls Env* env_target() const { return target_.env; } diff --git a/env/env.cc b/env/env.cc index 2137738c7b..9f259f3693 100644 --- a/env/env.cc +++ b/env/env.cc @@ -93,12 +93,15 @@ class LegacySystemClock : public SystemClock { return env_->TimeToString(time); } - std::string SerializeOptions(const ConfigOptions& /*config_options*/, - const std::string& /*prefix*/) const override { + Status SerializeOptions( + const ConfigOptions& /*config_options*/, + std::unordered_map* /*options*/) + const override { // We do not want the LegacySystemClock to appear in the serialized output. // This clock is an internal class for those who do not implement one and // would be part of the Env. As such, do not serialize it here. - return ""; + return Status::OK(); + ; } }; @@ -599,13 +602,16 @@ class LegacyFileSystemWrapper : public FileSystem { return status_to_io_status(target_->IsDirectory(path, is_dir)); } - std::string SerializeOptions(const ConfigOptions& /*config_options*/, - const std::string& /*prefix*/) const override { + Status SerializeOptions( + const ConfigOptions& /*config_options*/, + std::unordered_map* /*options*/) + const override { // We do not want the LegacyFileSystem to appear in the serialized output. // This clock is an internal class for those who do not implement one and // would be part of the Env. As such, do not serialize it here. - return ""; + return Status::OK(); } + private: Env* target_; }; @@ -1164,7 +1170,7 @@ const std::shared_ptr& Env::GetSystemClock() const { } namespace { static std::unordered_map sc_wrapper_type_info = { - {"target", + {Customizable::kTargetPropName(), OptionTypeInfo::AsCustomSharedPtr( 0, OptionVerificationType::kByName, OptionTypeFlags::kDontSerialize)}, }; @@ -1182,24 +1188,14 @@ Status SystemClockWrapper::PrepareOptions(const ConfigOptions& options) { return SystemClock::PrepareOptions(options); } -std::string SystemClockWrapper::SerializeOptions( - const ConfigOptions& config_options, const std::string& header) const { - auto parent = SystemClock::SerializeOptions(config_options, ""); - if (config_options.IsShallow() || target_ == nullptr || - target_->IsInstanceOf(SystemClock::kDefaultName())) { - return parent; - } else { - std::string result = header; - if (!StartsWith(parent, OptionTypeInfo::kIdPropName())) { - result.append(OptionTypeInfo::kIdPropName()).append("="); - } - result.append(parent); - if (!EndsWith(result, config_options.delimiter)) { - result.append(config_options.delimiter); - } - result.append("target=").append(target_->ToString(config_options)); - return result; +Status SystemClockWrapper::SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const { + if (!config_options.IsShallow() && target_ != nullptr && + !target_->IsInstanceOf(SystemClock::kDefaultName())) { + options->insert({kTargetPropName(), target_->ToString(config_options)}); } + return SystemClock::SerializeOptions(config_options, options); } static int RegisterBuiltinSystemClocks(ObjectLibrary& library, diff --git a/env/file_system.cc b/env/file_system.cc index 71fb4d5bc7..5807050c22 100644 --- a/env/file_system.cc +++ b/env/file_system.cc @@ -226,7 +226,7 @@ IOStatus ReadFileToString(FileSystem* fs, const std::string& fname, namespace { static std::unordered_map fs_wrapper_type_info = { - {"target", + {Customizable::kTargetPropName(), OptionTypeInfo::AsCustomSharedPtr( 0, OptionVerificationType::kByName, OptionTypeFlags::kDontSerialize)}, }; @@ -243,24 +243,14 @@ Status FileSystemWrapper::PrepareOptions(const ConfigOptions& options) { return FileSystem::PrepareOptions(options); } -std::string FileSystemWrapper::SerializeOptions( - const ConfigOptions& config_options, const std::string& header) const { - auto parent = FileSystem::SerializeOptions(config_options, ""); - if (config_options.IsShallow() || target_ == nullptr || - target_->IsInstanceOf(FileSystem::kDefaultName())) { - return parent; - } else { - std::string result = header; - if (!StartsWith(parent, OptionTypeInfo::kIdPropName())) { - result.append(OptionTypeInfo::kIdPropName()).append("="); - } - result.append(parent); - if (!EndsWith(result, config_options.delimiter)) { - result.append(config_options.delimiter); - } - result.append("target=").append(target_->ToString(config_options)); - return result; +Status FileSystemWrapper::SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const { + if (!config_options.IsShallow() && target_ != nullptr && + !target_->IsInstanceOf(FileSystem::kDefaultName())) { + options->insert({kTargetPropName(), target_->ToString(config_options)}); } + return FileSystem::SerializeOptions(config_options, options); } DirFsyncOptions::DirFsyncOptions() { reason = kDefault; } diff --git a/include/rocksdb/configurable.h b/include/rocksdb/configurable.h index a200d7e86c..0684d80620 100644 --- a/include/rocksdb/configurable.h +++ b/include/rocksdb/configurable.h @@ -343,8 +343,9 @@ class Configurable { std::string* bad_name) const; // Internal method to serialize options (ToString) // Classes may override this value to change its behavior. - virtual std::string SerializeOptions(const ConfigOptions& config_options, - const std::string& header) const; + virtual Status SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const; // Given a name (e.g. rocksdb.my.type.opt), returns the short name (opt) virtual std::string GetOptionName(const std::string& long_name) const; diff --git a/include/rocksdb/convenience.h b/include/rocksdb/convenience.h index 7ce676df0c..b3d20fcaa3 100644 --- a/include/rocksdb/convenience.h +++ b/include/rocksdb/convenience.h @@ -103,6 +103,13 @@ struct ConfigOptions { bool IsCheckEnabled(SanityLevel level) const { return (level > SanityLevel::kSanityLevelNone && level <= sanity_level); } + // Converts the map of options to a single string representation + std::string ToString( + const std::string& prefix, + const std::unordered_map& options) const; + // Converts the vector options to a single string representation + std::string ToString(char separator, + const std::vector& elems) const; }; diff --git a/include/rocksdb/customizable.h b/include/rocksdb/customizable.h index 076aca6590..f64ab3d847 100644 --- a/include/rocksdb/customizable.h +++ b/include/rocksdb/customizable.h @@ -57,6 +57,8 @@ class Customizable : public Configurable { public: ~Customizable() override {} + constexpr static const char* kTargetPropName() { return "target"; } + // Returns the name of this class of Customizable virtual const char* Name() const = 0; @@ -222,8 +224,9 @@ class Customizable : public Configurable { virtual const char* NickName() const { return ""; } // Given a name (e.g. rocksdb.my.type.opt), returns the short name (opt) std::string GetOptionName(const std::string& long_name) const override; - std::string SerializeOptions(const ConfigOptions& options, - const std::string& prefix) const override; + Status SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const override; }; } // namespace ROCKSDB_NAMESPACE diff --git a/include/rocksdb/env.h b/include/rocksdb/env.h index 62af602c62..f431e03cd5 100644 --- a/include/rocksdb/env.h +++ b/include/rocksdb/env.h @@ -1643,8 +1643,9 @@ class EnvWrapper : public Env { target_.env->SanitizeEnvOptions(env_opts); } Status PrepareOptions(const ConfigOptions& options) override; - std::string SerializeOptions(const ConfigOptions& config_options, - const std::string& header) const override; + Status SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const override; private: Target target_; diff --git a/include/rocksdb/file_system.h b/include/rocksdb/file_system.h index f725c87a4f..399c57e7ef 100644 --- a/include/rocksdb/file_system.h +++ b/include/rocksdb/file_system.h @@ -1514,8 +1514,9 @@ class FileSystemWrapper : public FileSystem { const Customizable* Inner() const override { return target_.get(); } Status PrepareOptions(const ConfigOptions& options) override; - std::string SerializeOptions(const ConfigOptions& config_options, - const std::string& header) const override; + Status SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const override; virtual IOStatus Poll(std::vector& io_handles, size_t min_completions) override { diff --git a/include/rocksdb/system_clock.h b/include/rocksdb/system_clock.h index 7ca92e54e3..68ef003800 100644 --- a/include/rocksdb/system_clock.h +++ b/include/rocksdb/system_clock.h @@ -103,8 +103,9 @@ class SystemClockWrapper : public SystemClock { } Status PrepareOptions(const ConfigOptions& options) override; - std::string SerializeOptions(const ConfigOptions& config_options, - const std::string& header) const override; + Status SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const override; const Customizable* Inner() const override { return target_.get(); } protected: diff --git a/include/rocksdb/utilities/options_type.h b/include/rocksdb/utilities/options_type.h index cd340ed596..0c3b4aa8b0 100644 --- a/include/rocksdb/utilities/options_type.h +++ b/include/rocksdb/utilities/options_type.h @@ -857,7 +857,12 @@ class OptionTypeInfo { static Status SerializeType( const ConfigOptions& config_options, const std::unordered_map& type_map, - const void* opt_addr, std::string* value); + const void* opt_addr, + std::unordered_map* options); + static Status TypeToString( + const ConfigOptions& config_options, const std::string& opt_name, + const std::unordered_map& type_map, + const void* opt_addr, std::string* result); // Serializes the input addr according to the map for the struct to value. // struct_name is the name of the struct option as registered @@ -1156,33 +1161,22 @@ Status SerializeVector(const ConfigOptions& config_options, const OptionTypeInfo& elem_info, char separator, const std::string& name, const std::vector& vec, std::string* value) { - std::string result; - ConfigOptions embedded = config_options; - embedded.delimiter = ";"; - int printed = 0; - for (const auto& elem : vec) { - std::string elem_str; - Status s = elem_info.Serialize(embedded, name, &elem, &elem_str); - if (!s.ok()) { - return s; - } else if (!elem_str.empty()) { - if (printed++ > 0) { - result += separator; - } - // If the element contains embedded separators, put it inside of brackets - if (elem_str.find(separator) != std::string::npos) { - result += "{" + elem_str + "}"; - } else { - result += elem_str; + if (vec.empty()) { + value->clear(); + } else { + std::vector opt_vec; + ConfigOptions embedded = config_options; + embedded.delimiter = ";"; + for (const auto& elem : vec) { + std::string elem_str; + Status s = elem_info.Serialize(embedded, name, &elem, &elem_str); + if (!s.ok()) { + return s; + } else if (!elem_str.empty()) { + opt_vec.emplace_back(elem_str); } } - } - if (result.find("=") != std::string::npos) { - *value = "{" + result + "}"; - } else if (printed > 1 && result.at(0) == '{') { - *value = "{" + result + "}"; - } else { - *value = result; + *value = config_options.ToString(separator, opt_vec); } return Status::OK(); } diff --git a/memory/memory_allocator.cc b/memory/memory_allocator.cc index d0de26b94d..2ea1c92e4f 100644 --- a/memory/memory_allocator.cc +++ b/memory/memory_allocator.cc @@ -15,8 +15,9 @@ namespace ROCKSDB_NAMESPACE { namespace { static std::unordered_map ma_wrapper_type_info = { - {"target", OptionTypeInfo::AsCustomSharedPtr( - 0, OptionVerificationType::kByName, OptionTypeFlags::kNone)}, + {Customizable::kTargetPropName(), + OptionTypeInfo::AsCustomSharedPtr( + 0, OptionVerificationType::kByName, OptionTypeFlags::kNone)}, }; static int RegisterBuiltinAllocators(ObjectLibrary& library, diff --git a/options/cf_options.cc b/options/cf_options.cc index 3480b17c96..ff3d385086 100644 --- a/options/cf_options.cc +++ b/options/cf_options.cc @@ -1157,7 +1157,8 @@ Status GetStringFromMutableCFOptions(const ConfigOptions& config_options, std::string* opt_string) { assert(opt_string); opt_string->clear(); - return OptionTypeInfo::SerializeType( - config_options, cf_mutable_options_type_info, &mutable_opts, opt_string); + return OptionTypeInfo::TypeToString(config_options, "", + cf_mutable_options_type_info, + &mutable_opts, opt_string); } } // namespace ROCKSDB_NAMESPACE diff --git a/options/configurable.cc b/options/configurable.cc index 5491336e0a..780c7caa2d 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -456,29 +456,34 @@ Status ConfigurableHelper::ConfigureOption( Status Configurable::GetOptionString(const ConfigOptions& config_options, std::string* result) const { + std::unordered_map options; assert(result); result->clear(); - return ConfigurableHelper::SerializeOptions(config_options, *this, "", - result); + Status s = + ConfigurableHelper::SerializeOptions(config_options, *this, &options); + if (s.ok()) { + *result = config_options.ToString("", options); + } + return s; } std::string Configurable::ToString(const ConfigOptions& config_options, const std::string& prefix) const { - std::string result = SerializeOptions(config_options, prefix); - if (result.empty() || result.find('=') == std::string::npos) { - return result; + std::unordered_map options; + Status status = SerializeOptions(config_options, &options); + Status s = SerializeOptions(config_options, &options); + assert(s.ok()); + if (s.ok()) { + return config_options.ToString(prefix, options); } else { - return "{" + result + "}"; + return ""; } } -std::string Configurable::SerializeOptions(const ConfigOptions& config_options, - const std::string& header) const { - std::string result; - Status s = ConfigurableHelper::SerializeOptions(config_options, *this, header, - &result); - assert(s.ok()); - return result; +Status Configurable::SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const { + return ConfigurableHelper::SerializeOptions(config_options, *this, options); } Status Configurable::GetOption(const ConfigOptions& config_options, @@ -517,45 +522,16 @@ Status ConfigurableHelper::GetOption(const ConfigOptions& config_options, return Status::NotFound("Cannot find option: ", short_name); } -Status ConfigurableHelper::SerializeOptions(const ConfigOptions& config_options, - const Configurable& configurable, - const std::string& prefix, - std::string* result) { - assert(result); +Status ConfigurableHelper::SerializeOptions( + const ConfigOptions& config_options, const Configurable& configurable, + std::unordered_map* options) { + assert(options); 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; - if (opt_info.ShouldSerialize()) { - std::string 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()) { - // = - result->append(prefix + opt_name + "=" + value + - config_options.delimiter); - } - } + Status s = OptionTypeInfo::SerializeType( + config_options, *(opt_iter.type_map), opt_iter.opt_ptr, options); + if (!s.ok()) { + return s; } } } diff --git a/options/configurable_helper.h b/options/configurable_helper.h index 5d409f82a4..dce4c715e8 100644 --- a/options/configurable_helper.h +++ b/options/configurable_helper.h @@ -133,10 +133,9 @@ class ConfigurableHelper { // @return OK If the options for this object wer successfully serialized. // @return InvalidArgument If one or more of the options could not be // serialized. - static Status SerializeOptions(const ConfigOptions& config_options, - const Configurable& configurable, - const std::string& prefix, - std::string* result); + static Status SerializeOptions( + const ConfigOptions& config_options, const Configurable& configurable, + std::unordered_map* options); // Internal method to list the option names for this object. // Classes may override this value to change its behavior. diff --git a/options/customizable.cc b/options/customizable.cc index 2f154d84c5..6a96ed82a7 100644 --- a/options/customizable.cc +++ b/options/customizable.cc @@ -46,28 +46,19 @@ Status Customizable::GetOption(const ConfigOptions& config_options, } } -std::string Customizable::SerializeOptions(const ConfigOptions& config_options, - const std::string& prefix) const { - std::string result; - std::string parent; - std::string id = GetId(); +Status Customizable::SerializeOptions( + const ConfigOptions& config_options, + std::unordered_map* options) const { + Status s; + auto id = GetId(); + options->insert({OptionTypeInfo::kIdPropName(), id}); + if (!config_options.IsShallow() && !id.empty()) { - parent = Configurable::SerializeOptions(config_options, ""); - } - if (parent.empty()) { - result = id; - } else { - result.append(prefix); - result.append(OptionTypeInfo::kIdPropName()); - result.append("="); - result.append(id); - result.append(config_options.delimiter); - result.append(parent); + s = Configurable::SerializeOptions(config_options, options); } - return result; + return s; } - bool Customizable::AreEquivalent(const ConfigOptions& config_options, const Configurable* other, std::string* mismatch) const { diff --git a/options/customizable_test.cc b/options/customizable_test.cc index b8c8b077a7..140c6df5c7 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -783,10 +783,10 @@ TEST_F(CustomizableTest, TestStringDepth) { std::string opt_str; shallow.depth = ConfigOptions::Depth::kDepthShallow; ASSERT_OK(c->GetOptionString(shallow, &opt_str)); - ASSERT_EQ(opt_str, "inner=a;"); + ASSERT_EQ(opt_str, "inner=a"); shallow.depth = ConfigOptions::Depth::kDepthDetailed; ASSERT_OK(c->GetOptionString(shallow, &opt_str)); - ASSERT_NE(opt_str, "inner=a;"); + ASSERT_NE(opt_str, "inner=a"); } // Tests that we only get a new customizable when it changes diff --git a/options/db_options.cc b/options/db_options.cc index 25672e38fa..f8d1b5b4df 100644 --- a/options/db_options.cc +++ b/options/db_options.cc @@ -544,22 +544,14 @@ static std::unordered_map addr); ConfigOptions embedded = opts; embedded.delimiter = ";"; - int printed = 0; + std::vector vec; for (const auto& listener : *listeners) { auto id = listener->GetId(); if (!id.empty()) { - std::string elem_str = listener->ToString(embedded, ""); - if (printed++ == 0) { - value->append("{"); - } else { - value->append(":"); - } - value->append(elem_str); + vec.push_back(listener->ToString(embedded, "")); } } - if (printed > 0) { - value->append("}"); - } + *value = opts.ToString(':', vec); return Status::OK(); }, nullptr}}, @@ -1118,7 +1110,8 @@ bool MutableDBOptionsAreEqual(const MutableDBOptions& this_options, Status GetStringFromMutableDBOptions(const ConfigOptions& config_options, const MutableDBOptions& mutable_opts, std::string* opt_string) { - return OptionTypeInfo::SerializeType( - config_options, db_mutable_options_type_info, &mutable_opts, opt_string); + return OptionTypeInfo::TypeToString(config_options, "", + db_mutable_options_type_info, + &mutable_opts, opt_string); } } // namespace ROCKSDB_NAMESPACE diff --git a/options/options_helper.cc b/options/options_helper.cc index 2734eebcf1..8417ca603d 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -12,6 +12,7 @@ #include #include "options/cf_options.h" +#include "options/configurable_helper.h" #include "options/db_options.h" #include "rocksdb/cache.h" #include "rocksdb/compaction_filter.h" @@ -39,6 +40,69 @@ ConfigOptions::ConfigOptions(const DBOptions& db_opts) : env(db_opts.env) { registry = ObjectRegistry::NewInstance(); } +std::string ConfigOptions::ToString( + const std::string& /*prefix*/, + const std::unordered_map& options) const { + std::string result; + std::string id; + for (const auto& it : options) { + if (it.first == OptionTypeInfo::kIdPropName()) { + id = it.second; + } else { + if (!result.empty()) { + result.append(delimiter); + } + // if (!prefix.empty()) { + // result.append(prefix); + // } + result.append(it.first); + result.append("="); + if (it.second.find('=') != std::string::npos && it.second[0] != '{') { + result.append("{" + it.second + "}"); + } else { + result.append(it.second); + } + } + } + if (id.empty()) { + return result; + } else if (result.empty()) { + return id; + } else { + std::string id_string = /*prefix + */ OptionTypeInfo::kIdPropName(); + id_string.append("="); + id_string.append(id); + return id_string + delimiter + result; + } +} + +std::string ConfigOptions::ToString( + char separator, const std::vector& elems) const { + std::string result; + int printed = 0; + for (const auto& elem : elems) { + if (printed++ > 0) { + result += separator; + } + if (elem.find(separator) != std::string::npos) { + // If the element contains embedded separators, put it inside of brackets + result.append("{" + elem + "}"); + } else if (elem.find("=") != std::string::npos) { + // If the element contains embedded options, put it inside of brackets + result.append("{" + elem + "}"); + } else { + result += elem; + } + } + if (result.find("=") != std::string::npos) { + return "{" + result + "}"; + } else if (printed > 1 && result.at(0) == '{') { + return "{" + result + "}"; + } else { + return result; + } +} + Status ValidateOptions(const DBOptions& db_opts, const ColumnFamilyOptions& cf_opts) { Status s; @@ -908,9 +972,12 @@ Status OptionTypeInfo::Parse(const ConfigOptions& config_options, return Status::NotSupported("Deserializing the option " + opt_name + " is not supported"); } else { + printf("MJR: Error parsing[%s][%s]\n", opt_name.c_str(), value.c_str()); return Status::InvalidArgument("Error parsing:", opt_name); } } catch (std::exception& e) { + printf("MJR: Caught exception parsing[%s][%s]=[%s]\n", opt_name.c_str(), + value.c_str(), e.what()); return Status::InvalidArgument("Error parsing " + opt_name + ":" + std::string(e.what())); } @@ -1061,18 +1128,35 @@ Status OptionTypeInfo::Serialize(const ConfigOptions& config_options, Status OptionTypeInfo::SerializeType( const ConfigOptions& config_options, const std::unordered_map& type_map, - const void* opt_addr, std::string* result) { + const void* opt_addr, + std::unordered_map* options) { Status status; for (const auto& iter : type_map) { std::string single; + const auto& opt_name = iter.first; const auto& opt_info = iter.second; if (opt_info.ShouldSerialize()) { - status = - opt_info.Serialize(config_options, iter.first, opt_addr, &single); + if (!config_options.mutable_options_only) { + status = + opt_info.Serialize(config_options, opt_name, opt_addr, &single); + } else if (opt_info.IsMutable()) { + ConfigOptions copy = config_options; + copy.mutable_options_only = false; + status = opt_info.Serialize(copy, opt_name, opt_addr, &single); + } 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)) { + status = + opt_info.Serialize(config_options, opt_name, opt_addr, &single); + } + } if (!status.ok()) { return status; - } else { - result->append(iter.first + "=" + single + config_options.delimiter); + } else if (!single.empty()) { + options->insert_or_assign(iter.first, single); } } } @@ -1091,13 +1175,9 @@ Status OptionTypeInfo::SerializeStruct( ConfigOptions embedded = config_options; embedded.delimiter = ";"; - // This option represents the entire struct - std::string result; - status = SerializeType(embedded, *struct_map, opt_addr, &result); + status = TypeToString(embedded, opt_name, *struct_map, opt_addr, value); if (!status.ok()) { return status; - } else { - *value = "{" + result + "}"; } } else if (StartsWith(opt_name, struct_name + ".")) { // This option represents a nested field in the struct (e.g, struct.field) @@ -1123,6 +1203,19 @@ Status OptionTypeInfo::SerializeStruct( return status; } +Status OptionTypeInfo::TypeToString( + const ConfigOptions& config_options, const std::string& prefix, + const std::unordered_map& type_map, + const void* opt_addr, std::string* result) { + assert(result); + std::unordered_map options; + Status s = SerializeType(config_options, type_map, opt_addr, &options); + if (s.ok()) { + *result = config_options.ToString(prefix, options); + } + return s; +} + template bool IsOptionEqual(const void* offset1, const void* offset2) { return (*static_cast(offset1) == *static_cast(offset2)); diff --git a/options/options_test.cc b/options/options_test.cc index f5bddc927a..6d9cd056dd 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -4818,7 +4818,7 @@ TEST_F(OptionTypeInfoTest, TestStaticType) { std::string str, mismatch; ASSERT_OK( - OptionTypeInfo::SerializeType(config_options, type_map, &opts, &str)); + OptionTypeInfo::TypeToString(config_options, "", type_map, &opts, &str)); ASSERT_FALSE(OptionTypeInfo::TypesAreEqual(config_options, type_map, &opts, ©, &mismatch)); ASSERT_OK(OptionTypeInfo::ParseType(config_options, str, type_map, ©)); From 983f42f4b6aba43f7fb11d0341f71402014bfae4 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 1 Aug 2023 13:40:59 -0400 Subject: [PATCH 2/7] First Pass at serializing only options that are changed --- include/rocksdb/convenience.h | 4 +- options/configurable.cc | 93 ++++++++++++++++++++++++++++++++--- options/configurable_helper.h | 8 +++ options/options_helper.cc | 22 ++------- options/options_test.cc | 25 ++++++++++ 5 files changed, 126 insertions(+), 26 deletions(-) diff --git a/include/rocksdb/convenience.h b/include/rocksdb/convenience.h index b3d20fcaa3..71f37beac2 100644 --- a/include/rocksdb/convenience.h +++ b/include/rocksdb/convenience.h @@ -93,13 +93,15 @@ struct ConfigOptions { // The object registry to use for this options std::shared_ptr registry; + // If set, only changes from this reference version will be serialized. + Configurable* compare_to = nullptr; + bool IsShallow() const { return depth == Depth::kDepthShallow; } bool IsDetailed() const { return depth == Depth::kDepthDetailed; } bool IsCheckDisabled() const { return sanity_level == SanityLevel::kSanityLevelNone; } - bool IsCheckEnabled(SanityLevel level) const { return (level > SanityLevel::kSanityLevelNone && level <= sanity_level); } diff --git a/options/configurable.cc b/options/configurable.cc index 780c7caa2d..aed09f708a 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -526,15 +526,77 @@ Status ConfigurableHelper::SerializeOptions( const ConfigOptions& config_options, const Configurable& configurable, std::unordered_map* options) { assert(options); - for (auto const& opt_iter : configurable.options_) { - if (opt_iter.type_map != nullptr) { - Status s = OptionTypeInfo::SerializeType( - config_options, *(opt_iter.type_map), opt_iter.opt_ptr, options); - if (!s.ok()) { - return s; + ConfigOptions copy = config_options; + auto compare_to = config_options.compare_to; + if (compare_to != nullptr && !MayBeEquivalent(configurable, *compare_to)) { + // If we are comparing this type to another, first see if the types + // are the same. If not, forget it + compare_to = nullptr; + } + + Status s; + for (size_t i = 0; i < configurable.options_.size(); i++) { + const auto& opt = configurable.options_[i]; + if (opt.type_map != nullptr) { + const auto opt_addr = opt.opt_ptr; + for (const auto& opt_iter : *(opt.type_map)) { + std::string single; + const auto& opt_name = opt_iter.first; + const auto& opt_info = opt_iter.second; + bool should_serialize = opt_info.ShouldSerialize(); + if (should_serialize && compare_to != nullptr) { + // This option should be serialized but there is a possiblity that it + // matches the default. Check to see if we really should serialize it + std::string mismatch; + if (opt_info.AreEqual(config_options, opt_name, opt_addr, + compare_to->options_[i].opt_ptr, &mismatch)) { + should_serialize = false; + } + } + if (should_serialize) { + if (compare_to != nullptr && opt_info.IsCustomizable()) { + copy.compare_to = opt_info.AsRawPointer( + compare_to->options_[i].opt_ptr); + } else { + copy.compare_to = compare_to; + } + s = SerializeOption(copy, opt_name, opt_info, opt_addr, &single); + + if (!s.ok()) { + return s; + } else if (!single.empty()) { + options->insert_or_assign(opt_name, single); + } + } + } + } + } + return s; +} + +Status ConfigurableHelper::SerializeOption(const ConfigOptions& config_options, + const std::string& opt_name, + const OptionTypeInfo& opt_info, + const void* opt_addr, + std::string* value) { + if (opt_info.ShouldSerialize()) { + if (!config_options.mutable_options_only) { + return opt_info.Serialize(config_options, opt_name, opt_addr, value); + } else if (opt_info.IsMutable()) { + ConfigOptions copy = config_options; + copy.mutable_options_only = false; + return opt_info.Serialize(copy, opt_name, opt_addr, 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)) { + return opt_info.Serialize(config_options, opt_name, opt_addr, value); } } } + value->clear(); return Status::OK(); } @@ -613,6 +675,25 @@ bool Configurable::OptionsAreEqual(const ConfigOptions& config_options, } } +bool ConfigurableHelper::MayBeEquivalent(const Configurable& this_one, + const Configurable& that_one) { + if (this_one.options_.size() != that_one.options_.size()) { + // The two types do not have the same number of registered options, + // therefore they cannot be the same. + return false; + } + + for (size_t i = 0; i < this_one.options_.size(); i++) { + const auto& this_opt = this_one.options_[i]; + const auto& that_opt = that_one.options_[i]; + if (this_opt.name != that_opt.name || + this_opt.type_map != that_opt.type_map) { + return false; + } + } + return true; +} + bool ConfigurableHelper::AreEquivalent(const ConfigOptions& config_options, const Configurable& this_one, const Configurable& that_one, diff --git a/options/configurable_helper.h b/options/configurable_helper.h index dce4c715e8..83e10598b8 100644 --- a/options/configurable_helper.h +++ b/options/configurable_helper.h @@ -137,6 +137,11 @@ class ConfigurableHelper { const ConfigOptions& config_options, const Configurable& configurable, std::unordered_map* options); + static Status SerializeOption(const ConfigOptions& config_options, + const std::string& opt_name, + const OptionTypeInfo& opt_info, + const void* opt_addr, std::string* value); + // Internal method to list the option names for this object. // Classes may override this value to change its behavior. // @see ListOptions for more details @@ -158,6 +163,9 @@ class ConfigurableHelper { const Configurable& that_one, std::string* mismatch); + static bool MayBeEquivalent(const Configurable& this_one, + const Configurable& that_one); + private: // Looks for the option specified by name in the RegisteredOptions. // This method traverses the types in the input options vector. If an entry diff --git a/options/options_helper.cc b/options/options_helper.cc index 8417ca603d..bbcf51194a 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -1130,29 +1130,13 @@ Status OptionTypeInfo::SerializeType( const std::unordered_map& type_map, const void* opt_addr, std::unordered_map* options) { - Status status; for (const auto& iter : type_map) { std::string single; const auto& opt_name = iter.first; const auto& opt_info = iter.second; if (opt_info.ShouldSerialize()) { - if (!config_options.mutable_options_only) { - status = - opt_info.Serialize(config_options, opt_name, opt_addr, &single); - } else if (opt_info.IsMutable()) { - ConfigOptions copy = config_options; - copy.mutable_options_only = false; - status = opt_info.Serialize(copy, opt_name, opt_addr, &single); - } 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)) { - status = - opt_info.Serialize(config_options, opt_name, opt_addr, &single); - } - } + Status status = ConfigurableHelper::SerializeOption( + config_options, opt_name, opt_info, opt_addr, &single); if (!status.ok()) { return status; } else if (!single.empty()) { @@ -1160,7 +1144,7 @@ Status OptionTypeInfo::SerializeType( } } } - return status; + return Status::OK(); } Status OptionTypeInfo::SerializeStruct( diff --git a/options/options_test.cc b/options/options_test.cc index 6d9cd056dd..1161247cee 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -2234,6 +2234,31 @@ TEST_F(OptionsTest, OptionsListenerTest) { ASSERT_OK(RocksDBOptionsParser::VerifyDBOptions(config_opts, orig, copy)); } +TEST_F(OptionsTest, DBOptionsSerializeChangedOptions) { + Options base_options, new_options; + Random rnd(301); + + ConfigOptions config; + DBOptions base; + DBOptions copy; + std::string opts_str; + ASSERT_OK(GetStringFromDBOptions(config, base, &opts_str)); + auto dbcfg = DBOptionsAsConfigurable(base); + config.compare_to = dbcfg.get(); + ASSERT_OK(GetStringFromDBOptions(config, copy, &opts_str)); + copy.paranoid_checks = false; + copy.max_background_compactions = 10; + ASSERT_OK(GetStringFromDBOptions(config, copy, &opts_str)); + + copy.file_checksum_gen_factory = GetFileChecksumGenCrc32cFactory(); + ASSERT_OK(GetStringFromDBOptions(config, copy, &opts_str)); + + base.file_checksum_gen_factory = copy.file_checksum_gen_factory; + dbcfg = DBOptionsAsConfigurable(base); + config.compare_to = dbcfg.get(); + ASSERT_OK(GetStringFromDBOptions(config, copy, &opts_str)); +} + const static std::string kCustomEnvName = "Custom"; const static std::string kCustomEnvProp = "env=" + kCustomEnvName; From 846dd2a1f295b3fea1fd27d90d4e8477db3f7a04 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 8 Aug 2023 13:59:31 -0400 Subject: [PATCH 3/7] Revert "First Pass at serializing only options that are changed" This reverts commit 983f42f4b6aba43f7fb11d0341f71402014bfae4. --- include/rocksdb/convenience.h | 4 +- options/configurable.cc | 93 +++-------------------------------- options/configurable_helper.h | 8 --- options/options_helper.cc | 22 +++++++-- options/options_test.cc | 25 ---------- 5 files changed, 26 insertions(+), 126 deletions(-) diff --git a/include/rocksdb/convenience.h b/include/rocksdb/convenience.h index 71f37beac2..b3d20fcaa3 100644 --- a/include/rocksdb/convenience.h +++ b/include/rocksdb/convenience.h @@ -93,15 +93,13 @@ struct ConfigOptions { // The object registry to use for this options std::shared_ptr registry; - // If set, only changes from this reference version will be serialized. - Configurable* compare_to = nullptr; - bool IsShallow() const { return depth == Depth::kDepthShallow; } bool IsDetailed() const { return depth == Depth::kDepthDetailed; } bool IsCheckDisabled() const { return sanity_level == SanityLevel::kSanityLevelNone; } + bool IsCheckEnabled(SanityLevel level) const { return (level > SanityLevel::kSanityLevelNone && level <= sanity_level); } diff --git a/options/configurable.cc b/options/configurable.cc index aed09f708a..780c7caa2d 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -526,77 +526,15 @@ Status ConfigurableHelper::SerializeOptions( const ConfigOptions& config_options, const Configurable& configurable, std::unordered_map* options) { assert(options); - ConfigOptions copy = config_options; - auto compare_to = config_options.compare_to; - if (compare_to != nullptr && !MayBeEquivalent(configurable, *compare_to)) { - // If we are comparing this type to another, first see if the types - // are the same. If not, forget it - compare_to = nullptr; - } - - Status s; - for (size_t i = 0; i < configurable.options_.size(); i++) { - const auto& opt = configurable.options_[i]; - if (opt.type_map != nullptr) { - const auto opt_addr = opt.opt_ptr; - for (const auto& opt_iter : *(opt.type_map)) { - std::string single; - const auto& opt_name = opt_iter.first; - const auto& opt_info = opt_iter.second; - bool should_serialize = opt_info.ShouldSerialize(); - if (should_serialize && compare_to != nullptr) { - // This option should be serialized but there is a possiblity that it - // matches the default. Check to see if we really should serialize it - std::string mismatch; - if (opt_info.AreEqual(config_options, opt_name, opt_addr, - compare_to->options_[i].opt_ptr, &mismatch)) { - should_serialize = false; - } - } - if (should_serialize) { - if (compare_to != nullptr && opt_info.IsCustomizable()) { - copy.compare_to = opt_info.AsRawPointer( - compare_to->options_[i].opt_ptr); - } else { - copy.compare_to = compare_to; - } - s = SerializeOption(copy, opt_name, opt_info, opt_addr, &single); - - if (!s.ok()) { - return s; - } else if (!single.empty()) { - options->insert_or_assign(opt_name, single); - } - } - } - } - } - return s; -} - -Status ConfigurableHelper::SerializeOption(const ConfigOptions& config_options, - const std::string& opt_name, - const OptionTypeInfo& opt_info, - const void* opt_addr, - std::string* value) { - if (opt_info.ShouldSerialize()) { - if (!config_options.mutable_options_only) { - return opt_info.Serialize(config_options, opt_name, opt_addr, value); - } else if (opt_info.IsMutable()) { - ConfigOptions copy = config_options; - copy.mutable_options_only = false; - return opt_info.Serialize(copy, opt_name, opt_addr, 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)) { - return opt_info.Serialize(config_options, opt_name, opt_addr, value); + for (auto const& opt_iter : configurable.options_) { + if (opt_iter.type_map != nullptr) { + Status s = OptionTypeInfo::SerializeType( + config_options, *(opt_iter.type_map), opt_iter.opt_ptr, options); + if (!s.ok()) { + return s; } } } - value->clear(); return Status::OK(); } @@ -675,25 +613,6 @@ bool Configurable::OptionsAreEqual(const ConfigOptions& config_options, } } -bool ConfigurableHelper::MayBeEquivalent(const Configurable& this_one, - const Configurable& that_one) { - if (this_one.options_.size() != that_one.options_.size()) { - // The two types do not have the same number of registered options, - // therefore they cannot be the same. - return false; - } - - for (size_t i = 0; i < this_one.options_.size(); i++) { - const auto& this_opt = this_one.options_[i]; - const auto& that_opt = that_one.options_[i]; - if (this_opt.name != that_opt.name || - this_opt.type_map != that_opt.type_map) { - return false; - } - } - return true; -} - bool ConfigurableHelper::AreEquivalent(const ConfigOptions& config_options, const Configurable& this_one, const Configurable& that_one, diff --git a/options/configurable_helper.h b/options/configurable_helper.h index 83e10598b8..dce4c715e8 100644 --- a/options/configurable_helper.h +++ b/options/configurable_helper.h @@ -137,11 +137,6 @@ class ConfigurableHelper { const ConfigOptions& config_options, const Configurable& configurable, std::unordered_map* options); - static Status SerializeOption(const ConfigOptions& config_options, - const std::string& opt_name, - const OptionTypeInfo& opt_info, - const void* opt_addr, std::string* value); - // Internal method to list the option names for this object. // Classes may override this value to change its behavior. // @see ListOptions for more details @@ -163,9 +158,6 @@ class ConfigurableHelper { const Configurable& that_one, std::string* mismatch); - static bool MayBeEquivalent(const Configurable& this_one, - const Configurable& that_one); - private: // Looks for the option specified by name in the RegisteredOptions. // This method traverses the types in the input options vector. If an entry diff --git a/options/options_helper.cc b/options/options_helper.cc index bbcf51194a..8417ca603d 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -1130,13 +1130,29 @@ Status OptionTypeInfo::SerializeType( const std::unordered_map& type_map, const void* opt_addr, std::unordered_map* options) { + Status status; for (const auto& iter : type_map) { std::string single; const auto& opt_name = iter.first; const auto& opt_info = iter.second; if (opt_info.ShouldSerialize()) { - Status status = ConfigurableHelper::SerializeOption( - config_options, opt_name, opt_info, opt_addr, &single); + if (!config_options.mutable_options_only) { + status = + opt_info.Serialize(config_options, opt_name, opt_addr, &single); + } else if (opt_info.IsMutable()) { + ConfigOptions copy = config_options; + copy.mutable_options_only = false; + status = opt_info.Serialize(copy, opt_name, opt_addr, &single); + } 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)) { + status = + opt_info.Serialize(config_options, opt_name, opt_addr, &single); + } + } if (!status.ok()) { return status; } else if (!single.empty()) { @@ -1144,7 +1160,7 @@ Status OptionTypeInfo::SerializeType( } } } - return Status::OK(); + return status; } Status OptionTypeInfo::SerializeStruct( diff --git a/options/options_test.cc b/options/options_test.cc index 1161247cee..6d9cd056dd 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -2234,31 +2234,6 @@ TEST_F(OptionsTest, OptionsListenerTest) { ASSERT_OK(RocksDBOptionsParser::VerifyDBOptions(config_opts, orig, copy)); } -TEST_F(OptionsTest, DBOptionsSerializeChangedOptions) { - Options base_options, new_options; - Random rnd(301); - - ConfigOptions config; - DBOptions base; - DBOptions copy; - std::string opts_str; - ASSERT_OK(GetStringFromDBOptions(config, base, &opts_str)); - auto dbcfg = DBOptionsAsConfigurable(base); - config.compare_to = dbcfg.get(); - ASSERT_OK(GetStringFromDBOptions(config, copy, &opts_str)); - copy.paranoid_checks = false; - copy.max_background_compactions = 10; - ASSERT_OK(GetStringFromDBOptions(config, copy, &opts_str)); - - copy.file_checksum_gen_factory = GetFileChecksumGenCrc32cFactory(); - ASSERT_OK(GetStringFromDBOptions(config, copy, &opts_str)); - - base.file_checksum_gen_factory = copy.file_checksum_gen_factory; - dbcfg = DBOptionsAsConfigurable(base); - config.compare_to = dbcfg.get(); - ASSERT_OK(GetStringFromDBOptions(config, copy, &opts_str)); -} - const static std::string kCustomEnvName = "Custom"; const static std::string kCustomEnvProp = "env=" + kCustomEnvName; From 8df931c7d62f43459098130cd3e8836de3058a84 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 8 Aug 2023 15:27:19 -0400 Subject: [PATCH 4/7] More use of SerializeOptions --- options/configurable.cc | 8 ++++---- options/customizable.cc | 14 ++++++-------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/options/configurable.cc b/options/configurable.cc index 780c7caa2d..51ee1a28cf 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -132,7 +132,7 @@ Status Configurable::ConfigureOptions( const ConfigOptions& config_options, const std::unordered_map& opts_map, std::unordered_map* unused) { - std::string curr_opts; + std::unordered_map curr_opts; Status s; if (!opts_map.empty()) { // There are options in the map. @@ -145,8 +145,8 @@ Status Configurable::ConfigureOptions( // If we are not ignoring unused, get the defaults in case we need to // reset copy.depth = ConfigOptions::kDepthDetailed; - copy.delimiter = "; "; - GetOptionString(copy, &curr_opts).PermitUncheckedError(); + ConfigurableHelper::SerializeOptions(copy, *this, &curr_opts) + .PermitUncheckedError(); } s = ConfigurableHelper::ConfigureOptions(copy, *this, opts_map, unused); @@ -160,7 +160,7 @@ Status Configurable::ConfigureOptions( 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(); + ConfigureFromMap(reset, curr_opts).PermitUncheckedError(); } return s; } diff --git a/options/customizable.cc b/options/customizable.cc index 6a96ed82a7..5aec45e94e 100644 --- a/options/customizable.cc +++ b/options/customizable.cc @@ -7,6 +7,7 @@ #include +#include "options/configurable_helper.h" #include "options/options_helper.h" #include "port/port.h" #include "rocksdb/convenience.h" @@ -94,14 +95,11 @@ Status Customizable::GetOptionsMap( if (status.ok() && customizable->IsInstanceOf(*id)) { // The new ID and the old ID match, so the objects are the same type. // Try to get the existing options, ignoring any errors - ConfigOptions embedded = config_options; - embedded.delimiter = ";"; - std::string curr_opts; - if (customizable->GetOptionString(embedded, &curr_opts).ok()) { - std::unordered_map curr_props; - if (StringToMap(curr_opts, &curr_props).ok()) { - props->insert(curr_props.begin(), curr_props.end()); - } + std::unordered_map curr_opts; + if (ConfigurableHelper::SerializeOptions(config_options, *customizable, + &curr_opts) + .ok()) { + props->insert(curr_opts.begin(), curr_opts.end()); } } } else { From c2ecfa1616286e4bc5e0e44e79bac34b37c917b9 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Mon, 21 Aug 2023 15:42:05 -0400 Subject: [PATCH 5/7] Serialize only the options that are changed Adds a "compare_to" to the ConfigOptions. If set, the serialize methods will only serialize values that do not match this value. --- include/rocksdb/convenience.h | 3 + options/configurable.cc | 107 ++++++++++++++++- options/configurable_helper.h | 8 ++ options/options_helper.cc | 51 ++------- options/options_test.cc | 210 ++++++++++++++++++++++++++++++++++ 5 files changed, 329 insertions(+), 50 deletions(-) diff --git a/include/rocksdb/convenience.h b/include/rocksdb/convenience.h index b3d20fcaa3..3951952ab5 100644 --- a/include/rocksdb/convenience.h +++ b/include/rocksdb/convenience.h @@ -93,6 +93,9 @@ struct ConfigOptions { // The object registry to use for this options std::shared_ptr registry; + // If set, only changes from this reference version will be serialized. + Configurable* compare_to = nullptr; + bool IsShallow() const { return depth == Depth::kDepthShallow; } bool IsDetailed() const { return depth == Depth::kDepthDetailed; } diff --git a/options/configurable.cc b/options/configurable.cc index 51ee1a28cf..1a58e7eee9 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -526,15 +526,91 @@ Status ConfigurableHelper::SerializeOptions( const ConfigOptions& config_options, const Configurable& configurable, std::unordered_map* options) { assert(options); - for (auto const& opt_iter : configurable.options_) { - if (opt_iter.type_map != nullptr) { - Status s = OptionTypeInfo::SerializeType( - config_options, *(opt_iter.type_map), opt_iter.opt_ptr, options); - if (!s.ok()) { - return s; + ConfigOptions copy = config_options; + auto compare_to = config_options.compare_to; + if (compare_to != nullptr && !MayBeEquivalent(configurable, *compare_to)) { + // If we are comparing this type to another, first see if the types + // are the same. If not, forget it + compare_to = nullptr; + } + + Status s; + for (size_t i = 0; i < configurable.options_.size(); i++) { + const auto& opt = configurable.options_[i]; + if (opt.type_map != nullptr) { + const auto opt_addr = opt.opt_ptr; + for (const auto& opt_iter : *(opt.type_map)) { + std::string single; + const auto& opt_name = opt_iter.first; + const auto& opt_info = opt_iter.second; + bool should_serialize = opt_info.ShouldSerialize(); + if (should_serialize && compare_to != nullptr) { + // This option should be serialized but there is a possiblity that it + // matches the default. Check to see if we really should serialize it + std::string mismatch; + if (opt_info.IsConfigurable() && + opt_info.IsEnabled(OptionTypeFlags::kStringNameOnly) && + !config_options.IsDetailed()) { + // If it is a Configurable name-only and we are not printing the + // details, then compare loosely + copy.sanity_level = ConfigOptions::kSanityLevelLooselyCompatible; + if (opt_info.AreEqual(copy, opt_name, opt_addr, + compare_to->options_[i].opt_ptr, &mismatch)) { + should_serialize = false; + } else { + printf("MJR: Options do not match[%s]=%s\n", opt_name.c_str(), + mismatch.c_str()); + } + copy.sanity_level = config_options.sanity_level; + } else if (opt_info.AreEqual(config_options, opt_name, opt_addr, + compare_to->options_[i].opt_ptr, + &mismatch)) { + should_serialize = false; + } + } + if (should_serialize) { + if (compare_to != nullptr && opt_info.IsCustomizable()) { + copy.compare_to = opt_info.AsRawPointer( + compare_to->options_[i].opt_ptr); + } else { + copy.compare_to = compare_to; + } + s = SerializeOption(copy, opt_name, opt_info, opt_addr, &single); + if (!s.ok()) { + return s; + } else if (!single.empty()) { + options->insert_or_assign(opt_name, single); + } + } + } + } + } + return s; +} + +Status ConfigurableHelper::SerializeOption(const ConfigOptions& config_options, + const std::string& opt_name, + const OptionTypeInfo& opt_info, + const void* opt_addr, + std::string* value) { + if (opt_info.ShouldSerialize()) { + if (!config_options.mutable_options_only) { + return opt_info.Serialize(config_options, opt_name, opt_addr, value); + } else if (opt_info.IsMutable()) { + ConfigOptions copy = config_options; + copy.mutable_options_only = false; + return opt_info.Serialize(copy, opt_name, opt_addr, 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)) { + return opt_info.Serialize(config_options, opt_name, opt_addr, value); } } } + value->clear(); return Status::OK(); } @@ -580,6 +656,25 @@ Status ConfigurableHelper::ListOptions( // //******************************************************************************* +bool ConfigurableHelper::MayBeEquivalent(const Configurable& this_one, + const Configurable& that_one) { + if (this_one.options_.size() != that_one.options_.size()) { + // The two types do not have the same number of registered options, + // therefore they cannot be the same. + return false; + } + + for (size_t i = 0; i < this_one.options_.size(); i++) { + const auto& this_opt = this_one.options_[i]; + const auto& that_opt = that_one.options_[i]; + if (this_opt.name != that_opt.name || + this_opt.type_map != that_opt.type_map) { + return false; + } + } + return true; +} + bool Configurable::AreEquivalent(const ConfigOptions& config_options, const Configurable* other, std::string* name) const { diff --git a/options/configurable_helper.h b/options/configurable_helper.h index dce4c715e8..83e10598b8 100644 --- a/options/configurable_helper.h +++ b/options/configurable_helper.h @@ -137,6 +137,11 @@ class ConfigurableHelper { const ConfigOptions& config_options, const Configurable& configurable, std::unordered_map* options); + static Status SerializeOption(const ConfigOptions& config_options, + const std::string& opt_name, + const OptionTypeInfo& opt_info, + const void* opt_addr, std::string* value); + // Internal method to list the option names for this object. // Classes may override this value to change its behavior. // @see ListOptions for more details @@ -158,6 +163,9 @@ class ConfigurableHelper { const Configurable& that_one, std::string* mismatch); + static bool MayBeEquivalent(const Configurable& this_one, + const Configurable& that_one); + private: // Looks for the option specified by name in the RegisteredOptions. // This method traverses the types in the input options vector. If an entry diff --git a/options/options_helper.cc b/options/options_helper.cc index 8417ca603d..b8a7dc4fc0 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -972,12 +972,9 @@ Status OptionTypeInfo::Parse(const ConfigOptions& config_options, return Status::NotSupported("Deserializing the option " + opt_name + " is not supported"); } else { - printf("MJR: Error parsing[%s][%s]\n", opt_name.c_str(), value.c_str()); return Status::InvalidArgument("Error parsing:", opt_name); } } catch (std::exception& e) { - printf("MJR: Caught exception parsing[%s][%s]=[%s]\n", opt_name.c_str(), - value.c_str(), e.what()); return Status::InvalidArgument("Error parsing " + opt_name + ":" + std::string(e.what())); } @@ -1130,29 +1127,13 @@ Status OptionTypeInfo::SerializeType( const std::unordered_map& type_map, const void* opt_addr, std::unordered_map* options) { - Status status; for (const auto& iter : type_map) { std::string single; const auto& opt_name = iter.first; const auto& opt_info = iter.second; if (opt_info.ShouldSerialize()) { - if (!config_options.mutable_options_only) { - status = - opt_info.Serialize(config_options, opt_name, opt_addr, &single); - } else if (opt_info.IsMutable()) { - ConfigOptions copy = config_options; - copy.mutable_options_only = false; - status = opt_info.Serialize(copy, opt_name, opt_addr, &single); - } 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)) { - status = - opt_info.Serialize(config_options, opt_name, opt_addr, &single); - } - } + Status status = ConfigurableHelper::SerializeOption( + config_options, opt_name, opt_info, opt_addr, &single); if (!status.ok()) { return status; } else if (!single.empty()) { @@ -1160,7 +1141,7 @@ Status OptionTypeInfo::SerializeType( } } } - return status; + return Status::OK(); } Status OptionTypeInfo::SerializeStruct( @@ -1290,7 +1271,8 @@ bool OptionTypeInfo::AreEqual(const ConfigOptions& config_options, const void* const that_ptr, std::string* mismatch) const { auto level = GetSanityLevel(); - if (!config_options.IsCheckEnabled(level)) { + if (config_options.compare_to == nullptr && + !config_options.IsCheckEnabled(level)) { return true; // If the sanity level is not being checked, skip it } if (this_ptr == nullptr || that_ptr == nullptr) { @@ -1317,7 +1299,8 @@ bool OptionTypeInfo::AreEqual(const ConfigOptions& config_options, } else if (this_config != nullptr && that_config != nullptr) { std::string bad_name; bool matches; - if (level < config_options.sanity_level) { + if (config_options.compare_to == nullptr && + level < config_options.sanity_level) { ConfigOptions copy = config_options; copy.sanity_level = level; matches = this_config->AreEquivalent(copy, that_config, &bad_name); @@ -1399,26 +1382,6 @@ bool OptionTypeInfo::StructsAreEqual( return matches; } -bool MatchesOptionsTypeFromMap( - const ConfigOptions& config_options, - const std::unordered_map& type_map, - const void* const this_ptr, const void* const that_ptr, - std::string* mismatch) { - for (auto& pair : type_map) { - // We skip checking deprecated variables as they might - // contain random values since they might not be initialized - if (config_options.IsCheckEnabled(pair.second.GetSanityLevel())) { - if (!pair.second.AreEqual(config_options, pair.first, this_ptr, that_ptr, - mismatch) && - !pair.second.AreEqualByName(config_options, pair.first, this_ptr, - that_ptr)) { - return false; - } - } - } - return true; -} - bool OptionTypeInfo::AreEqualByName(const ConfigOptions& config_options, const std::string& opt_name, const void* const this_ptr, diff --git a/options/options_test.cc b/options/options_test.cc index 6d9cd056dd..0696124f74 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -30,6 +30,7 @@ #include "util/random.h" #include "util/stderr_logger.h" #include "util/string_util.h" +#include "utilities/merge_operators.h" #include "utilities/merge_operators/bytesxor.h" #include "utilities/merge_operators/sortlist.h" #include "utilities/merge_operators/string_append/stringappend.h" @@ -2234,6 +2235,215 @@ TEST_F(OptionsTest, OptionsListenerTest) { ASSERT_OK(RocksDBOptionsParser::VerifyDBOptions(config_opts, orig, copy)); } +static void TestDBOptionsChanged(const std::string& base_opts, + const std::string& changed_opts, + size_t changed) { + DBOptions base, copy; + ConfigOptions config; + std::string opts_str; + std::unordered_map opts_map; + + std::string trace_message; + if (!base_opts.empty()) { + trace_message = base_opts + "||" + changed_opts; + } else { + trace_message = changed_opts; + } + SCOPED_TRACE(trace_message.c_str()); + + if (!base_opts.empty()) { + ASSERT_OK(GetDBOptionsFromString(config, base, base_opts, &base)); + } + ASSERT_OK(GetDBOptionsFromString(config, base, changed_opts, ©)); + + auto dbcfg = DBOptionsAsConfigurable(base); + config.compare_to = dbcfg.get(); + config.sanity_level = ConfigOptions::kSanityLevelExactMatch; + config.depth = ConfigOptions::kDepthDetailed; + config.ignore_unknown_options = false; + config.ignore_unsupported_options = false; + + ASSERT_OK(GetStringFromDBOptions(config, copy, &opts_str)); + if (changed > 0) { + ASSERT_OK(StringToMap(opts_str, &opts_map)); + ASSERT_EQ(opts_map.size(), changed); + ASSERT_OK(GetDBOptionsFromMap(config, base, opts_map, &base)); + ASSERT_OK(RocksDBOptionsParser::VerifyDBOptions(config, base, copy)); + + dbcfg = DBOptionsAsConfigurable(base); + config.compare_to = dbcfg.get(); + ASSERT_OK(GetStringFromDBOptions(config, copy, &opts_str)); + } + ASSERT_EQ(opts_str, ""); +} + +TEST_F(OptionsTest, DBOptionsSerializeChangedOptions) { + TestDBOptionsChanged("", "", 0UL); // No changes + TestDBOptionsChanged("", "max_background_compactions=10", 1UL); + TestDBOptionsChanged( + "", "paranoid_checks=false; max_background_compactions=10", 2UL); + TestDBOptionsChanged( + "", "file_checksum_gen_factory=FileChecksumGenCrc32cFactory", 1UL); + TestDBOptionsChanged("file_checksum_gen_factory=FileChecksumGenCrc32cFactory", + "file_checksum_gen_factory=nullptr", 1UL); + TestDBOptionsChanged("file_checksum_gen_factory=FileChecksumGenCrc32cFactory", + "file_checksum_gen_factory=FileChecksumGenCrc32cFactory", + 0UL); +} + +static void TestCFOptionsChanged(const std::string& base_opts, + const std::string& changed_opts, + size_t changed) { + ColumnFamilyOptions base, copy; + ConfigOptions config; + std::string opts_str; + std::unordered_map opts_map; + + std::string trace_message; + if (!base_opts.empty()) { + trace_message = base_opts + "||" + changed_opts; + } else { + trace_message = changed_opts; + } + SCOPED_TRACE(trace_message.c_str()); + + if (!base_opts.empty()) { + ASSERT_OK(GetColumnFamilyOptionsFromString(config, base, base_opts, &base)); + } + ASSERT_OK( + GetColumnFamilyOptionsFromString(config, base, changed_opts, ©)); + + auto cfcfg = CFOptionsAsConfigurable(base); + config.compare_to = cfcfg.get(); + config.sanity_level = ConfigOptions::kSanityLevelExactMatch; + config.depth = ConfigOptions::kDepthDetailed; + config.ignore_unknown_options = false; + config.ignore_unsupported_options = false; + + ASSERT_OK(GetStringFromColumnFamilyOptions(config, copy, &opts_str)); + printf("MJR:[%s]=[%s]\n", trace_message.c_str(), opts_str.c_str()); + if (changed > 0) { + ASSERT_OK(StringToMap(opts_str, &opts_map)); + ASSERT_EQ(opts_map.size(), changed); + ASSERT_OK(GetColumnFamilyOptionsFromMap(config, base, opts_map, &base)); + ASSERT_OK(RocksDBOptionsParser::VerifyCFOptions(config, base, copy)); + + cfcfg = CFOptionsAsConfigurable(base); + config.compare_to = cfcfg.get(); + ASSERT_OK(GetStringFromColumnFamilyOptions(config, copy, &opts_str)); + } + ASSERT_EQ(opts_str, ""); +} + +TEST_F(OptionsTest, CFOptionsSerializeChangedOptions) { + TestCFOptionsChanged("", "", 0UL); // No changes + TestCFOptionsChanged("", "compression=kXpressCompression", 1UL); + TestCFOptionsChanged("compression=kXpressCompression", + "compression=kSnappyCompression", 1UL); + TestCFOptionsChanged("", "comparator=rocksdb.ReverseBytewiseComparator", 1UL); + TestCFOptionsChanged("comparator=rocksdb.ReverseBytewiseComparator", + "comparator=leveldb.BytewiseComparator", 1UL); + TestCFOptionsChanged( + "", "merge_operator={id=StringAppendOperator;delimiter=|}", 1UL); + TestCFOptionsChanged("merge_operator={id=StringAppendOperator;delimiter=|}", + "merge_operator=nullptr", 1UL); + TestCFOptionsChanged("merge_operator={id=StringAppendOperator;delimiter=|}", + "merge_operator={id=StringAppendOperator;delimiter=%}", + 1UL); + TestCFOptionsChanged("", "block_based_table_factory={block_size=8192}", 1UL); + TestCFOptionsChanged("", "table_factory=PlainTable", 1UL); +} + +TEST_F(OptionsTest, SerializeChangedOptionsNameOnly) { + ColumnFamilyOptions base, copy; + ConfigOptions config; + std::string opts_str; + std::unordered_map opts_map; + + config.ignore_unknown_options = false; + config.ignore_unsupported_options = false; + config.depth = ConfigOptions::kDepthDefault; + + // Compare the table factories directly + copy.table_factory.reset(NewPlainTableFactory()); + opts_str = copy.table_factory->ToString(config); + + config.compare_to = base.table_factory.get(); + ASSERT_EQ(opts_str, copy.table_factory->ToString(config)); + + copy.table_factory.reset(NewBlockBasedTableFactory()); + auto bbto = copy.table_factory->GetOptions(); + ASSERT_NE(bbto, nullptr); + bbto->block_size = 123456; + auto tf_str = copy.table_factory->ToString(config); + ASSERT_OK(StringToMap(tf_str, &opts_map)); + ASSERT_EQ(opts_map.size(), 2); // block_size+id + opts_map.clear(); + + auto cfcfg = CFOptionsAsConfigurable(base); + config.compare_to = cfcfg.get(); + ASSERT_OK(GetStringFromColumnFamilyOptions(config, copy, &opts_str)); + ASSERT_EQ(opts_str, ""); + + // When not doing detailed, table factories do not compare sub-options + // so the options match + + config.depth = ConfigOptions::kDepthDetailed; + ASSERT_OK(GetStringFromColumnFamilyOptions(config, copy, &opts_str)); + ASSERT_OK(StringToMap(opts_str, &opts_map)); + ASSERT_EQ(opts_map.size(), 1); // table factory + ASSERT_EQ(tf_str, opts_map.begin()->second.c_str()); +} + +TEST_F(OptionsTest, SerializeChangedOptionsCompareLoosely) { + ColumnFamilyOptions base, copy; + ConfigOptions config; + std::string opts_str; + + config.ignore_unknown_options = false; + config.ignore_unsupported_options = false; + config.sanity_level = ConfigOptions::kSanityLevelLooselyCompatible; + + ASSERT_OK(MergeOperator::CreateFromString( + config, "id=StringAppendOperator;delimiter=|", &base.merge_operator)); + ASSERT_OK(MergeOperator::CreateFromString( + config, "{id=StringAppendOperator;delimiter=|}", ©.merge_operator)); + + auto copy_str = copy.merge_operator->ToString(config); + auto cfcfg = CFOptionsAsConfigurable(base); + + // Compare the merge operators directly + ASSERT_EQ(copy_str, base.merge_operator->ToString(config)); + config.compare_to = base.merge_operator.get(); + ASSERT_OK(copy.merge_operator->GetOptionString(config, &opts_str)); + ASSERT_EQ(opts_str, ""); + + config.compare_to = cfcfg.get(); + ASSERT_OK(GetStringFromColumnFamilyOptions(config, copy, &opts_str)); + ASSERT_EQ(opts_str, ""); + + ASSERT_OK(MergeOperator::CreateFromString( + config, "{id=StringAppendOperator;delimiter=%}", ©.merge_operator)); + copy_str = copy.merge_operator->ToString(config); + + config.compare_to = base.merge_operator.get(); + ASSERT_OK(copy.merge_operator->GetOptionString(config, &opts_str)); + ASSERT_EQ(opts_str, "delimiter=%"); + + ASSERT_OK(GetStringFromColumnFamilyOptions(config, base, &opts_str)); + + config.compare_to = cfcfg.get(); + ASSERT_OK(GetStringFromColumnFamilyOptions(config, base, &opts_str)); + ASSERT_EQ(opts_str, ""); + + std::unordered_map opts_map; + config.sanity_level = ConfigOptions::kSanityLevelExactMatch; + ASSERT_OK(GetStringFromColumnFamilyOptions(config, copy, &opts_str)); + ASSERT_OK(StringToMap(opts_str, &opts_map)); + ASSERT_EQ(opts_map.size(), 1UL); + ASSERT_EQ(copy_str, opts_map.begin()->second.c_str()); +} + const static std::string kCustomEnvName = "Custom"; const static std::string kCustomEnvProp = "env=" + kCustomEnvName; From 6c8e296b6e4213fbe96ffd09d269f50bf1544285 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Thu, 19 Oct 2023 09:20:28 -0400 Subject: [PATCH 6/7] Recover from merge to main --- options/configurable.cc | 6 ++++-- options/configurable_helper.h | 14 ++++++++++++++ options/options_helper.cc | 22 +++------------------- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/options/configurable.cc b/options/configurable.cc index 826e45c125..e9d6ccc809 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -587,11 +587,13 @@ Status ConfigurableHelper::SerializeOptions(const ConfigOptions& config_options, } else { copy.compare_to = compare_to; } - s = SerializeOption(copy, opt_name, opt_info, opt_addr, &single); + s = SerializeOption(copy, + OptionTypeInfo::MakePrefix(prefix, opt_name), + opt_info, opt_addr, &single); if (!s.ok()) { return s; } else if (!single.empty()) { - options->insert_or_assign(opt_name, single); + props->insert_or_assign(opt_name, single); } } } diff --git a/options/configurable_helper.h b/options/configurable_helper.h index 7b635e8a83..cca7aa1884 100644 --- a/options/configurable_helper.h +++ b/options/configurable_helper.h @@ -152,6 +152,19 @@ class ConfigurableHelper { const std::string& prefix, OptionProperties* props); + // Serializes a single option to its string representation + // @param opt_name The name of the option + // @param opt_info The type and related information of the option + // @param opt_addr The address of the option + // @param value The string representation of the option. + // @return OK If the options for this object wer successfully serialized. + // @return InvalidArgument If one or more of the options could not be + // serialized. + static Status SerializeOption(const ConfigOptions& config_options, + const std::string& opt_name, + const OptionTypeInfo& opt_info, + const void* opt_addr, std::string* value); + // Internal method to list the option names for this object. // Classes may override this value to change its behavior. // @see ListOptions for more details @@ -173,6 +186,7 @@ class ConfigurableHelper { const Configurable& that_one, std::string* mismatch); + // Checks to see if the two Configurable classes may be equivalent static bool MayBeEquivalent(const Configurable& this_one, const Configurable& that_one); diff --git a/options/options_helper.cc b/options/options_helper.cc index efc1a252df..40cd020e63 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -1136,30 +1136,14 @@ Status OptionTypeInfo::SerializeType( const ConfigOptions& config_options, const std::string& prefix, const std::unordered_map& type_map, const void* opt_addr, OptionProperties* props) { - Status status; for (const auto& iter : type_map) { std::string single; const auto& opt_name = iter.first; const auto& opt_info = iter.second; if (opt_info.ShouldSerialize()) { - if (!config_options.mutable_options_only) { - status = opt_info.Serialize( - config_options, MakePrefix(prefix, opt_name), opt_addr, &single); - } else if (opt_info.IsMutable()) { - ConfigOptions copy = config_options; - copy.mutable_options_only = false; - status = opt_info.Serialize(copy, MakePrefix(prefix, opt_name), - opt_addr, &single); - } 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)) { - status = opt_info.Serialize( - config_options, MakePrefix(prefix, opt_name), opt_addr, &single); - } - } + Status status = ConfigurableHelper::SerializeOption( + config_options, MakePrefix(prefix, opt_name), opt_info, opt_addr, + &single); if (!status.ok()) { return status; } else if (!single.empty()) { From 6df249d8c2beac9196e322ffbbc1eda9dfa5d2b5 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Mon, 30 Oct 2023 07:57:49 -0400 Subject: [PATCH 7/7] Updated HISTORY file --- HISTORY.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/HISTORY.md b/HISTORY.md index 65c4c8242e..560a46086c 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -7,7 +7,7 @@ ### New Features * Non-Blocking Manual Compaction (CompactRange()) - Support non-blocking manual compactions by setting a new CompactRangeOptions option (async_completion_cb). When set, the CompactRange() call will return control to the caller immediately. The manual compaction iteslf will be performed in an internally created thread. The manual compaction will ALWAYS call the specified callback upon completion and provide the completion status (#597). * Change the internal Configurable API SerializeOptions to return UserProperties (instead of the final string representation). Added ToString methods to the ConfigurableOptions class to complete the serialization of Options properties. - +* Added ConfigOptions::compare_to. When set, this value causes only values that have been changed to be part of the serialized output. ### Enhancements * Unit Testing: Expose the disallow_trivial_move flag in the MoveFilesToLevel testing utility (#677).