Change Configurable::SerializeOptions method to return a map

HISTORY.md

@@ -6,6 +6,7 @@ Fix RepeatableThread to work properly with on thread start callback feature (htt
 
 ### 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.
 
 ### Enhancements
 * Unit Testing: Expose the disallow_trivial_move flag in the MoveFilesToLevel testing utility (#677).

db/compaction/compaction_service_job.cc

@@ -693,8 +693,8 @@ static std::unordered_map<std::string, OptionTypeInfo> cs_result_type_info = {
         const auto status_obj = static_cast<const Status*>(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);
+  OptionProperties props;
+  Status s =
+      OptionTypeInfo::SerializeType(cf, "", cs_input_type_info, this, &props);
+  if (s.ok()) {
+    output->append(cf.ToString("", props) + 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);
+  OptionProperties props;
+  Status s =
+      OptionTypeInfo::SerializeType(cf, "", cs_result_type_info, this, &props);
+  if (s.ok()) {
+    output->append(cf.ToString("", props) + cf.delimiter);
+  }
+  return s;
 }
 
 #ifndef NDEBUG

db/db_basic_test.cc

@@ -2197,6 +2197,8 @@ TEST_F(DBBasicTest, DBSetThreadAffinity) {
     CPU_ZERO(&cpuset);
     CPU_SET(0, &cpuset);
     pthread_setaffinity_np(thr, sizeof(cpu_set_t), &cpuset);
+#else
+    (void)thr;
 #endif
   };
   options.on_thread_start_callback =

env/composite_env.cc

@@ -391,7 +391,7 @@ Status CompositeEnv::NewDirectory(const std::string& name,
 
 namespace {
 static std::unordered_map<std::string, OptionTypeInfo> env_wrapper_type_info = {
-    {"target",
+    {Customizable::kTargetPropName(),
      OptionTypeInfo(0, OptionType::kUnknown, OptionVerificationType::kByName,
                     OptionTypeFlags::kDontSerialize)
          .SetParseFunc([](const ConfigOptions& opts,
@@ -482,14 +482,16 @@ 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, const std::string& prefix,
+    OptionProperties* props) const {
   if (target_.env != nullptr && target_.env != Env::Default()) {
-    options.append("target=");
-    options.append(target_.env->ToString(config_options));
+    props->insert({kTargetPropName(),
+                   target_.env->ToString(
+                       config_options,
+                       OptionTypeInfo::MakePrefix(prefix, kTargetPropName()))});
   }
-  return options;
+  return CompositeEnv::SerializeOptions(config_options, prefix, props);
 }
 
 EnvWrapper::EnvWrapper(Env* t) : target_(t) {
@@ -511,24 +513,17 @@ 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,
+                                    const std::string& prefix,
+                                    OptionProperties* props) const {
+  if (!config_options.IsShallow() && target_.env != nullptr &&
+      target_.env != Env::Default()) {
+    props->insert({kTargetPropName(),
+                   target_.env->ToString(
+                       config_options,
+                       OptionTypeInfo::MakePrefix(prefix, kTargetPropName()))});
+  }
+  return Env::SerializeOptions(config_options, prefix, props);
 }
 
 }  // namespace ROCKSDB_NAMESPACE

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,
+                          const std::string& prefix,
+                          OptionProperties* props) const override;
 
   // Return the target to which this Env forwards all calls
   Env* env_target() const { return target_.env; }

env/env.cc

@@ -93,12 +93,14 @@ 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*/,
+                          const std::string& /*prefix*/,
+                          OptionProperties* /*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 +601,15 @@ 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*/,
+                          const std::string& /*prefix*/,
+                          OptionProperties* /*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 +1168,7 @@ const std::shared_ptr<SystemClock>& Env::GetSystemClock() const {
 }
 namespace {
 static std::unordered_map<std::string, OptionTypeInfo> sc_wrapper_type_info = {
-    {"target",
+    {Customizable::kTargetPropName(),
      OptionTypeInfo::AsCustomSharedPtr<SystemClock>(
          0, OptionVerificationType::kByName, OptionTypeFlags::kDontSerialize)},
 };
@@ -1182,24 +1186,17 @@ 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,
+                                            const std::string& prefix,
+                                            OptionProperties* props) const {
+  if (!config_options.IsShallow() && target_ != nullptr &&
+      !target_->IsInstanceOf(SystemClock::kDefaultName())) {
+    props->insert(
+        {kTargetPropName(),
+         target_->ToString(config_options, OptionTypeInfo::MakePrefix(
+                                               prefix, kTargetPropName()))});
+  }
+  return SystemClock::SerializeOptions(config_options, prefix, props);
 }
 
 static int RegisterBuiltinSystemClocks(ObjectLibrary& library,

env/file_system.cc

@@ -226,7 +226,7 @@ IOStatus ReadFileToString(FileSystem* fs, const std::string& fname,
 
 namespace {
 static std::unordered_map<std::string, OptionTypeInfo> fs_wrapper_type_info = {
-    {"target",
+    {Customizable::kTargetPropName(),
      OptionTypeInfo::AsCustomSharedPtr<FileSystem>(
          0, OptionVerificationType::kByName, OptionTypeFlags::kDontSerialize)},
 };
@@ -243,24 +243,17 @@ 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,
+                                           const std::string& prefix,
+                                           OptionProperties* props) const {
+  if (!config_options.IsShallow() && target_ != nullptr &&
+      !target_->IsInstanceOf(FileSystem::kDefaultName())) {
+    props->insert(
+        {kTargetPropName(),
+         target_->ToString(config_options, OptionTypeInfo::MakePrefix(
+                                               prefix, kTargetPropName()))});
   }
+  return FileSystem::SerializeOptions(config_options, prefix, props);
 }
 
 DirFsyncOptions::DirFsyncOptions() { reason = kDefault; }

include/rocksdb/configurable.h

@@ -20,7 +20,9 @@
 namespace ROCKSDB_NAMESPACE {
 class Logger;
 class ObjectRegistry;
+class OptionProperties;
 class OptionTypeInfo;
+
 struct ColumnFamilyOptions;
 struct ConfigOptions;
 struct DBOptions;
@@ -343,8 +345,14 @@ 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;
+  // @param config_options Controls how the options are being matched
+  // @param prefix A string that may be prepended to every option.
+  // @param props Filled with the serialized name-value pairs of the options
+  //
+  // Returns OK on success or an error status if serialized failed.
+  virtual Status SerializeOptions(const ConfigOptions& config_options,
+                                  const std::string& prefix,
+                                  OptionProperties* props) 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;

include/rocksdb/convenience.h

@@ -18,7 +18,7 @@ namespace ROCKSDB_NAMESPACE {
 class Env;
 class Logger;
 class ObjectRegistry;
-
+class OptionProperties;
 struct ColumnFamilyOptions;
 struct DBOptions;
 struct Options;
@@ -103,6 +103,14 @@ struct ConfigOptions {
   bool IsCheckEnabled(SanityLevel level) const {
     return (level > SanityLevel::kSanityLevelNone && level <= sanity_level);
   }
+
+  // Converts the properties to a single string representation
+  std::string ToString(const std::string& prefix,
+                       const OptionProperties& props) const;
+
+  // Converts the vector options to a single string representation
+  std::string ToString(char separator,
+                       const std::vector<std::string>& elems) const;
 };
 
 

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,
+                          const std::string& prefix,
+                          OptionProperties* props) const override;
 };
 
 }  // namespace ROCKSDB_NAMESPACE

include/rocksdb/env.h

@@ -1658,8 +1658,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,
+                          const std::string& prefix,
+                          OptionProperties* props) const override;
 
  private:
   Target target_;

include/rocksdb/file_system.h

@@ -1528,8 +1528,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,
+                          const std::string& prefix,
+                          OptionProperties* props) const override;
 
   virtual IOStatus Poll(std::vector<void*>& io_handles,
                         size_t min_completions) override {

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,
+                          const std::string& prefix,
+                          OptionProperties* props) const override;
   const Customizable* Inner() const override { return target_.get(); }
 
  protected: