db bench: usability of the pinning policy parameter #720

udi-speedb · 2023-10-16T14:26:51Z

No description provided.

mrambacher · 2023-10-17T12:50:13Z

tools/db_bench_tool.cc

@@ -818,7 +819,27 @@ DEFINE_bool(
    " Note `cache_index_and_filter_blocks` must be true for this option to have"
    " any effect.");

-DEFINE_string(pinning_policy, "", "URI for registry TablePinningPolicy");
+DEFINE_bool(scoped_pinning_policy, false, "Use Speedb's Scoped Pinning Policy");


Can we please leave this as a string? Boolean parameters make it much harder to do validation and more alternatives.

The string could be compared to the kClassName or the like...

The whole point was to make it easy for users such as @erez-speedb to command db_bench to use scoped pinning policy. A boolean achieves just that. The whole point of this PR is usability and making the lives of the user easy.

And what validations are you referring to in this context?

If we add a third pinning policy and the value is a boolean, then you can only set one of the booleans. A string is simpler and more expandable/adaptable. Look at the code for handling different Envs to see an example.

I believe the problem before was the string that was required was too complex. This code simplifies the string (and provides validation) by separating the fields that were in one string into multiple parameters.

And is it really that much harder to say --pinning_policy=scoped than --scoped_pinning_policy?

I have reverted it to a string but the values are NOT the Class names (which are long and hard to remember. @erez-speedb complained about the length of the command line as it is).
The values (at the moment are: "default" and "scoped")

I strongly suggest making the values class names and changing the class name if you do not like it. Using name/nickname of "scoped" seems very reasonable to me. The MergeOperators with names like "MergeSortOperator" and "sortlist" are great examples of names and nicknames that make sense

mrambacher · 2023-10-17T12:52:26Z

tools/db_bench_tool.cc

+    if (FLAGS_cost_write_buffer_to_cache) {
+      if (FLAGS_db_write_buffer_size > FLAGS_cache_size) {
+        ErrorExit("--cache_size must be >= --db_write_buffer_size");
+      }


What does this have to do with the pinning policy?

Indirectly. These flags are used later in the calculation of the pinnning capacity.

mrambacher · 2023-10-17T12:53:55Z

tools/db_bench_tool.cc

  ValidateMetadataCacheOptions();
+  ValidatePinningRelatedOptions();


Aren't these two mutually exclusive? The MetadataCacheOptions are not used if PinningRelatedOptions are used

Not necessarily. When using the default policy ValidateMetadataCacheOptions is applicable. Each one specific flags and their inter-dependencies.
What do you propose instead?

Are the MetadataCacheOptions validation a problem with db_bench or the code in general?

I guess leave it for now. Not worth the hassle.

mrambacher · 2023-10-18T18:25:31Z

plugin/speedb/pinning_policy/scoped_pinning_policy.h

 //
 class ScopedPinningPolicy : public RecordingPinningPolicy {
 public:
  ScopedPinningPolicy();
  ScopedPinningPolicy(const ScopedPinningOptions& options);

  static const char* kClassName() { return "speedb_scoped_pinning_policy"; }
-  static const char* kNickName() { return "speedb.ScopedPinningPolicy"; }


I am fine with the name "ScopedPinning" but "Scoped" or "scoped" would be acceptable too. The fact the class is PinningPolicy implies "Pinning"

mrambacher · 2023-10-18T18:27:37Z

db_stress_tool/db_stress_gflags.cc

@@ -890,7 +891,12 @@ DEFINE_string(fs_uri, "",
              " with --env_uri."
              " Creates a default environment with the specified filesystem.");

-DEFINE_string(pinning_policy, "", "URI for registry TablePinningPolicy");
+DEFINE_string(pinning_policy,
+              ROCKSDB_NAMESPACE::DefaultPinningPolicy::kNickName(),


Why not leave empty be the default?

Allows the user to clearly see the actual pinning policy used by default.
Please see also my answer to your comment below.

Setting this to a legal value rather than empty makes it much harder to tell if it is updated. If it is empty, then the system will use default. If not, it should use the one that is set.

mrambacher · 2023-10-18T18:28:18Z

db_stress_tool/db_stress_gflags.cc

+              "The options are: "
+              "'DefaultPinning': Default RocksDB's pinning polcy. "
+              "'ScopedPinning': Speedb's Scoped pinning policy.");


Why explicitly state what the options are? If more were added via a plugin, this would be wrong. It also makes it hard to maintain.

Obviously, however, the top priority here IMO is to make it easy for the users (and seeing the strings to use does just that). That is a purpose of a help string.
Adding a new policy is a rare event. Such a policy would probably have its own set of configuration flags that should be added anyway to db_bench, so, I believe that the developer adding the policy would update the help string / default if necessary.

…bench

udi-speedb · 2023-10-19T03:21:51Z

@mrambacher - I have pushed another commit. Hopefully the last one. Please see if there is anything that you really think is important enough. Otherwise, please approve. Thanks

Yuval-Ariel · 2023-10-19T07:41:16Z

@udi-speedb note that theres a conflict

udi-speedb · 2023-10-19T07:59:15Z

@udi-speedb note that theres a conflict

Thanks. Resolved

mrambacher

thanks

mrambacher · 2023-10-19T15:15:55Z

db_stress_tool/db_stress_gflags.cc

@@ -890,7 +891,12 @@ DEFINE_string(fs_uri, "",
              " with --env_uri."
              " Creates a default environment with the specified filesystem.");

-DEFINE_string(pinning_policy, "", "URI for registry TablePinningPolicy");
+DEFINE_string(pinning_policy,
+              ROCKSDB_NAMESPACE::DefaultPinningPolicy::kNickName(),


Setting this to a legal value rather than empty makes it much harder to tell if it is updated. If it is empty, then the system will use default. If not, it should use the one that is set.

…y-of-the-pinning-policy-parameter

udi-speedb linked an issue Oct 16, 2023 that may be closed by this pull request

db_bench usability of the pinning policy parameter #687

Closed

udi-speedb force-pushed the 687-db_bench-usability-of-the-pinning-policy-parameter branch from 5470540 to 64dc9ed Compare October 16, 2023 14:28

udi-speedb requested a review from Yuval-Ariel October 17, 2023 11:26

db_bench: Add support for individual scoped pinning policy parameters

6ceb098

udi-speedb force-pushed the 687-db_bench-usability-of-the-pinning-policy-parameter branch from 64dc9ed to 6ceb098 Compare October 17, 2023 11:30

Guyme requested review from mrambacher and removed request for Yuval-Ariel October 17, 2023 12:15

mrambacher reviewed Oct 17, 2023

View reviewed changes

udi-speedb added 4 commits October 18, 2023 05:30

CR Updates

80e1cd9

Fix typos in db_bench flags' help strings

6aa7661

CR Updates #2

44655df

Adapt crash_test to db_bench pinning policy changes

46fb820

udi-speedb requested a review from mrambacher October 18, 2023 15:40

mrambacher reviewed Oct 18, 2023

View reviewed changes

Rename (simplify) the nicknames of pinning policies to be used in db_…

fa2dc8b

…bench

mrambacher approved these changes Oct 19, 2023

View reviewed changes

udi-speedb added 2 commits October 19, 2023 18:52

Merge remote-tracking branch 'origin/main' into 687-db_bench-usabilit…

8ac0b06

…y-of-the-pinning-policy-parameter

Fix minor compilation error

52156ff

udi-speedb force-pushed the 687-db_bench-usability-of-the-pinning-policy-parameter branch from 6f5965d to 52156ff Compare October 19, 2023 16:36

udi-speedb merged commit 8c68304 into main Oct 19, 2023
2 checks passed

udi-speedb added a commit that referenced this pull request Nov 23, 2023

db bench: usability of the pinning policy parameter (#720)

5a36c39

udi-speedb added a commit that referenced this pull request Dec 6, 2023

db bench: usability of the pinning policy parameter (#720)

19038c4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

db bench: usability of the pinning policy parameter #720

db bench: usability of the pinning policy parameter #720

udi-speedb commented Oct 16, 2023

mrambacher Oct 17, 2023

mrambacher Oct 17, 2023

udi-speedb Oct 17, 2023

mrambacher Oct 17, 2023

udi-speedb Oct 18, 2023

mrambacher Oct 18, 2023

mrambacher Oct 17, 2023

udi-speedb Oct 17, 2023

mrambacher Oct 17, 2023

udi-speedb Oct 17, 2023

mrambacher Oct 17, 2023

mrambacher Oct 18, 2023

udi-speedb Oct 19, 2023

mrambacher Oct 18, 2023

udi-speedb Oct 19, 2023

mrambacher Oct 19, 2023

mrambacher Oct 18, 2023

udi-speedb Oct 19, 2023

udi-speedb commented Oct 19, 2023

Yuval-Ariel commented Oct 19, 2023

udi-speedb commented Oct 19, 2023

mrambacher left a comment

mrambacher Oct 19, 2023

		ValidateMetadataCacheOptions();
		ValidatePinningRelatedOptions();

db bench: usability of the pinning policy parameter #720

db bench: usability of the pinning policy parameter #720

Conversation

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

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 19, 2023

Yuval-Ariel commented Oct 19, 2023

udi-speedb commented Oct 19, 2023

mrambacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment