Change Configurable::SerializeOptions method to return a map #619

mrambacher · 2023-08-01T17:51:42Z

Changes the Configurable::SerializeOptions method to return a map of options. This map is equivalent to the input map for the ConfigureOptions method. By returning a map, it will be possible in the future to add different output representations (such as DUMP, JSON, Properties) formats to the string representations of the configurable objects.

This PR is a first-step to a few additional changes. For example, is a gateway to:

Only serializing changed options;
Allowing different string formats for options (e.g. Default, Properties, Log/Dump, JSON, etc)

Converts an object to its serialized name-value properties.

This reverts commit 983f42f.

udi-speedb · 2023-08-09T04:03:14Z

@mrambacher - Please update when you have completed updating the branch. I will review only once the branch has all of the contents you intend to merge. Thanks

mrambacher · 2023-10-13T19:19:22Z

A couple of points:

I do not know if it is better to return/use a map or a vector of string pairs. Vectors may be slightly easier to work with in some cases and more efficient. They would also preserve the order if that is important (whereas map order is more random). The Configure methods currently use maps (but perhaps an internal wrapper method should be created to use the same type)
I was going to create something like a typedef/using for the map but could not decide on a good name. Feel free to suggest something as the current type is pretty long to type...

udi-speedb · 2023-10-14T02:20:33Z

@mrambacher - Do you still have content to push to the PR?

mrambacher · 2023-10-14T03:02:16Z

@mrambacher - Do you still have content to push to the PR?

@udi-speedb I just made an update to use the Properties type. I am done now pending comments.

udi-speedb · 2023-10-16T16:41:38Z

I may be missing something, but how is the addition of the prefix parameter part of this PR?

udi-speedb · 2023-10-15T10:10:19Z

include/rocksdb/customizable.h

@@ -57,6 +57,8 @@ class Customizable : public Configurable {
 public:
  ~Customizable() override {}



Please document the purpose of this method / constants

udi-speedb · 2023-10-15T10:12:34Z

include/rocksdb/customizable.h

@@ -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,


Please document this method.
Specifically, please describe prefix & return value (props)

udi-speedb · 2023-10-15T10:16:55Z

include/rocksdb/convenience.h

@@ -23,6 +23,8 @@ struct ColumnFamilyOptions;
 struct DBOptions;
 struct Options;

+using Properties = std::unordered_map<std::string, std::string>;


The type name (Properties) is too general to be part of the 'rocksdb' namespace. It should be under a more contrained scope or namespace.

Renamed to OptionProperties

udi-speedb · 2023-10-15T10:19:17Z

include/rocksdb/convenience.h

@@ -103,6 +105,12 @@ 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,


Please document the use / purpose of the prefix parameter. However, I see that the prefix parameter is not actually in use in the implementation. Is it really needed?

The prefix parameter was there previously (called header) and is needed by some of the formats (like Log format)

options/options_helper.cc

include/rocksdb/configurable.h

options/cf_options.cc

udi-speedb · 2023-10-16T16:43:14Z

options/cf_options.cc

-  return OptionTypeInfo::SerializeType(
-      config_options, cf_mutable_options_type_info, &mutable_opts, opt_string);
+  return OptionTypeInfo::TypeToString(config_options, "",
+                                      cf_mutable_options_type_info,


Why have you moved to TypeToString()?

SerializeType now returns a OptionsProperties set. TypeToString converts the Properties to a string. There were multiple places that needed this functionality, so I put it under a single API/method.

udi-speedb · 2023-10-16T16:48:34Z

include/rocksdb/convenience.h

@@ -103,6 +105,12 @@ struct ConfigOptions {
  bool IsCheckEnabled(SanityLevel level) const {
    return (level > SanityLevel::kSanityLevelNone && level <= sanity_level);
  }
+  // Converts the properties to a single string representation


How are these 2 methods related to this PR?

The ToString methods are used to convert the UserProperties to their string representation. Prior, the code was "hard-coded" in the various places

udi-speedb · 2023-10-16T16:51:09Z

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;


There is a [[maybe_unused]] attribute

This is here because of a general build issue on Macs. I have fixed this in many PRs now. Hopefully one will take...

udi-speedb · 2023-10-16T17:09:49Z

And please update HISTORY.md and add the license text in all the existing files you have modified.

Also added/updated comments.

mrambacher · 2023-10-17T12:31:32Z

I may be missing something, but how is the addition of the prefix parameter part of this PR?

The prefix parameter to SerializeOptions was there before -- the variable was called "header"

Changes the Configurable::SerializeOptions method to return a map of options. This map is equivalent to the input map for the ConfigureOptions method. By returning a map, it will be possible in the future to add different output representations (such as DUMP, JSON, Properties) formats to the string representations of the configurable objects. This PR is a first-step to a few additional changes. For example, is a gateway to: Only serializing changed options; Allowing different string formats for options (e.g. Default, Properties, Log/Dump, JSON, etc)

mrambacher added 2 commits July 31, 2023 16:02

Add a SerializeOptions method to Configurable

6dd7279

Converts an object to its serialized name-value properties.

First Pass at serializing only options that are changed

983f42f

mrambacher added the enhancement New feature or request label Aug 1, 2023

mrambacher requested a review from udi-speedb August 1, 2023 17:51

mrambacher self-assigned this Aug 1, 2023

mrambacher added 2 commits August 8, 2023 13:59

Revert "First Pass at serializing only options that are changed"

846dd2a

This reverts commit 983f42f.

More use of SerializeOptions

8df931c

mrambacher changed the title ~~WIP: Allow Serialization of only options that have changed~~ Change the Configurable::SerializeOptions method to return a map Aug 9, 2023

mrambacher changed the title ~~Change the Configurable::SerializeOptions method to return a map~~ Change Configurable::SerializeOptions method to return a map Aug 11, 2023

mrambacher mentioned this pull request Aug 21, 2023

Serialize only options that have changed from the some default #648

Merged

mrambacher added 2 commits September 1, 2023 11:03

Fix unnecessary (extra) call

b373e4f

Merge remote-tracking branch 'upstream/main' into ConfigureFromVector

a7fb784

mrambacher added 2 commits October 13, 2023 15:47

Merge remote-tracking branch 'upstream/main' into ConfigureFromVector

70392c9

Add prefix to Serialize; Add Properties typedef

7a80d1c

udi-speedb requested changes Oct 16, 2023

View reviewed changes

Rename Properties to OptionProperties

aded70d

Also added/updated comments.

Add comments

6efcbb9

udi-speedb self-requested a review October 17, 2023 18:43

udi-speedb approved these changes Oct 17, 2023

View reviewed changes

mrambacher added 4 commits October 17, 2023 17:06

Merge remote-tracking branch 'upstream/main' into ConfigureFromVector

829da3f

Add license and copyright

253cf62

Add Copyright notices

303570f

make format

9dd077a

Yuval-Ariel merged commit dc39061 into speedb-io:main Oct 19, 2023
1 check passed

mrambacher mentioned this pull request Nov 1, 2023

Log Improvement: Options for only the first 10 column families are reported to the log #430

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Configurable::SerializeOptions method to return a map #619

Change Configurable::SerializeOptions method to return a map #619

mrambacher commented Aug 1, 2023 •

edited

udi-speedb commented Aug 9, 2023

mrambacher commented Oct 13, 2023

udi-speedb commented Oct 14, 2023

mrambacher commented Oct 14, 2023

udi-speedb commented Oct 16, 2023

udi-speedb Oct 15, 2023

udi-speedb Oct 15, 2023

udi-speedb Oct 15, 2023

mrambacher Oct 17, 2023

udi-speedb Oct 15, 2023

mrambacher Oct 17, 2023

udi-speedb Oct 16, 2023

mrambacher Oct 17, 2023

udi-speedb Oct 16, 2023

mrambacher Oct 17, 2023

udi-speedb Oct 16, 2023

mrambacher Oct 17, 2023

udi-speedb commented Oct 16, 2023

mrambacher commented Oct 17, 2023

		@@ -57,6 +57,8 @@ class Customizable : public Configurable {
		public:
		~Customizable() override {}

Change Configurable::SerializeOptions method to return a map #619

Change Configurable::SerializeOptions method to return a map #619

Conversation

mrambacher commented Aug 1, 2023 • edited

udi-speedb commented Aug 9, 2023

mrambacher commented Oct 13, 2023

udi-speedb commented Oct 14, 2023

mrambacher commented Oct 14, 2023

udi-speedb commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

udi-speedb commented Oct 16, 2023

mrambacher commented Oct 17, 2023

mrambacher commented Aug 1, 2023 •

edited