feat: Add max_batching_delay option to CommitRequest. #11939

nginsberg-google · 2023-06-21T23:16:46Z

This change is

snippet-bot · 2023-06-21T23:16:54Z

Here is the summary of changes.

You are about to add 1 region tag.

google/cloud/spanner/samples/samples.cc:3954, tag spanner_commit_with_batching_delay

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

devbww · 2023-06-21T23:34:04Z

/gcbrun

coryan · 2023-06-21T23:38:33Z

google/cloud/spanner/commit_options.h

@@ -70,6 +71,16 @@ class CommitOptions {
    return request_priority_;
  }

+  CommitOptions& set_max_batching_delay(
+      absl::optional<absl::Duration> max_batching_delay) {


We should not use absl::Duration in a public API.

Done, changed to type int and renamed to max_batching_delay_ms.

Sorry for the confusion. As @coryan pointed out, "should not use absl::Duration in a public API" means to use std::chrono::duration instead.

coryan · 2023-06-21T23:39:47Z

google/cloud/spanner/samples/samples.cc

@@ -3802,6 +3802,16 @@ void QueryInformationSchemaDatabaseOptions(
 }
 // [END spanner_query_information_schema_database_options]

+// [START spanner_set_batching_delay]
+void CommitWithBatchingDelay(google::cloud::spanner::Client client,


This example does not show me anything that I did not know from reading the prototype. Do we really need it?

Can we change it to do something less obvious?

In particular, batching_delay is unused.

devbww · 2023-06-21T23:52:06Z

google/cloud/spanner/commit_options.h

 #include "absl/types/optional.h"
+#include "absl/time/time.h"


See the checkers build failure for problems with include ordering and indentation.

When I look at that build failure it appears empty. When I click "see more information" it looks like I don't have permission. Am I missing something?

There are other checkers failures too. Let me know if you still can't see them and we can try to fix that and/or quote them for you.

devbww · 2023-06-21T23:53:09Z

google/cloud/spanner/internal/connection_impl.cc

@@ -1017,6 +1017,12 @@ StatusOr<spanner::CommitResult> ConnectionImpl::CommitImpl(
  request.mutable_request_options()->set_priority(
      ProtoRequestPriority(params.options.request_priority()));

+  if (params.options.max_batching_delay().has_value()) {
+    *request.mutable_max_batching_delay() =


There is no max_batching_delay field in the published CommitRequest.

This field is currently hidden by a TRUSTED_TESTER label. My impression was the order in which I was supposed to do things was:

Get this PR (and an equivalent one for other client libraries) approved

Remove TT label

Merge these PRs.

I have changed my version of this codebase to include the newly generated code when the TT label is removed, but do not have permission to add it to a branch on this codebase. Please let me know if I'm doing something wrong.

That sequence will not work for C++. For complicated reasons¹, the C++ clients can only compile against public protos. In this case the public version of CommitRequest does not have the field you want to use.

Footnotes

The C++ client libraries cannot enforce what version of Protobuf our customers will use, and the generated *.pb.h and *.pb.cc files can only work with the version of Protobuf that generated them. Therefore, these files are not included in the Cloud C++ source, the customer generates these files, and the customer only has access to the public proto files. ↩

Hm, maybe I'm missing something. Step (2) in the sequence I listed will make the CommitRequest proto contain this field. Once we have a PR for each client library that the client library team is happy with, I will remove the visibility label internally and then the public version of CommitRequest will contain the max_batching_delay field. At that point we will be able to merge this PR. Is there a different sequence of steps the client library team usually takes?

You need at least:

2.5) Update the googleapis SHA in this repository to use a newer version.
2.6) Get the code to actually compile and pass under CI.

Is there a different sequence of steps the client library team usually takes?

I usually don't start writing code in C++ until the protos are published. I think @devbww has created patches for the proto files and then tweaks the build scripts to apply these patches (there are placeholders in CMake and Bazel to support this). For obvious reasons that work happens in branches that do not get published to PRs or any public location.

As @coryan points out, we really need the new protos published publicly, and google-cloud-cpp reconfigured to use the new protos, before worrying too much about actually using those new protos.

That said, we're happy to comment on the state of the PR, but having it in a non-compilable state make things more difficult.

devbww · 2023-06-21T23:53:51Z

google/cloud/spanner/samples/samples.cc

@@ -3802,6 +3802,16 @@ void QueryInformationSchemaDatabaseOptions(
 }
 // [END spanner_query_information_schema_database_options]

+// [START spanner_set_batching_delay]
+void CommitWithBatchingDelay(google::cloud::spanner::Client client,


In particular, batching_delay is unused.

devbww · 2023-06-21T23:55:28Z

google/cloud/spanner/samples/samples.cc

+void CommitWithBatchingDelay(google::cloud::spanner::Client client,
+			     google::cloud::spanner::Mutations mutations,
+			     absl::Duration batching_delay) {
+  google::cloud::spanner::CommitOptions options;


CommitOptions is a deprecated part of the public API. Whatever is needed here should be expressed in terms of google::cloud::Options, and then by calling another Commit() overload.

I think I've done this. This field is much like RequestPriorityOption, and I've pretty much copied it. I don't think we need to overload Commit() again, as the current versions all take an Options field. Please take a look and let me know if I've messed this up.

All the current overloads take an Options argument, not a CommitOptions. The latter is deprecated, and we shouldn't be changing it. Whatever is needed here should only be expressed in terms of google::cloud::Options.

I'm sorry ... my "only" was too strict. We still need your modifications to CommitOptions as they are used internally (and for mocking). But we don't want to use CommitOptions as part of any sample. Instead, the sample should use MaxBatchingDelayOption directly.

devbww · 2023-06-21T23:56:38Z

google/cloud/spanner/samples/samples.cc

@@ -3802,6 +3802,16 @@ void QueryInformationSchemaDatabaseOptions(
 }
 // [END spanner_query_information_schema_database_options]

+// [START spanner_set_batching_delay]


This tag does not appear in the published list. If we're making up a new one, it should at least include "commit".

I've changed the tag. This is new - is there somewhere I need to add it that I've missed?

Firstly, it looks like you only updated the START tag, and not the END one.

But to the bigger issue, the point of these tags is to delimit regions that will be scraped into the Cloud Spanner documentation, so introducing a tag requires some cross-team coordination. For example, here are all the defined Spanner tags containing "commit": http://devrel/snippets?pageSize=100&sort=region_tag&direction=asc&product_id=9&region_tag=%25commit%25

Normally the tech writers would introduce the tag when they document the feature, otherwise it has no purpose.

devbww · 2023-06-21T23:57:04Z

google/cloud/spanner/samples/samples.cc

@@ -3802,6 +3802,16 @@ void QueryInformationSchemaDatabaseOptions(
 }
 // [END spanner_query_information_schema_database_options]

+// [START spanner_set_batching_delay]
+void CommitWithBatchingDelay(google::cloud::spanner::Client client,


The samples should be executed, but no one calls this.

coryan · 2023-07-21T13:48:12Z

@nginsberg-google do you think you will have time to work on this PR? If not, I think we will just close it.

nginsberg-google · 2023-07-26T20:45:15Z

Yes, I will still work on this PR. Sorry for the delay. Please don't close it :)

nginsberg-google

Appreciate the detailed review. This is now ready for rereview - PTAL. Thanks!

nginsberg-google · 2023-07-26T22:04:10Z

google/cloud/spanner/commit_options.h

 #include "absl/types/optional.h"
+#include "absl/time/time.h"


When I look at that build failure it appears empty. When I click "see more information" it looks like I don't have permission. Am I missing something?

nginsberg-google · 2023-07-26T22:07:32Z

google/cloud/spanner/commit_options.h

@@ -70,6 +71,16 @@ class CommitOptions {
    return request_priority_;
  }

+  CommitOptions& set_max_batching_delay(
+      absl::optional<absl::Duration> max_batching_delay) {


Done, changed to type int and renamed to max_batching_delay_ms.

nginsberg-google · 2023-07-26T22:11:03Z

google/cloud/spanner/internal/connection_impl.cc

@@ -1017,6 +1017,12 @@ StatusOr<spanner::CommitResult> ConnectionImpl::CommitImpl(
  request.mutable_request_options()->set_priority(
      ProtoRequestPriority(params.options.request_priority()));

+  if (params.options.max_batching_delay().has_value()) {
+    *request.mutable_max_batching_delay() =


This field is currently hidden by a TRUSTED_TESTER label. My impression was the order in which I was supposed to do things was:

Get this PR (and an equivalent one for other client libraries) approved

Remove TT label

Merge these PRs.

I have changed my version of this codebase to include the newly generated code when the TT label is removed, but do not have permission to add it to a branch on this codebase. Please let me know if I'm doing something wrong.

nginsberg-google · 2023-07-26T22:13:09Z

google/cloud/spanner/samples/samples.cc

@@ -3802,6 +3802,16 @@ void QueryInformationSchemaDatabaseOptions(
 }
 // [END spanner_query_information_schema_database_options]

+// [START spanner_set_batching_delay]
+void CommitWithBatchingDelay(google::cloud::spanner::Client client,


nginsberg-google · 2023-07-26T22:41:43Z

google/cloud/spanner/samples/samples.cc

@@ -3802,6 +3802,16 @@ void QueryInformationSchemaDatabaseOptions(
 }
 // [END spanner_query_information_schema_database_options]

+// [START spanner_set_batching_delay]


I've changed the tag. This is new - is there somewhere I need to add it that I've missed?

nginsberg-google · 2023-07-26T22:46:40Z

google/cloud/spanner/samples/samples.cc

@@ -3802,6 +3802,16 @@ void QueryInformationSchemaDatabaseOptions(
 }
 // [END spanner_query_information_schema_database_options]

+// [START spanner_set_batching_delay]
+void CommitWithBatchingDelay(google::cloud::spanner::Client client,


nginsberg-google · 2023-07-26T22:48:17Z

google/cloud/spanner/samples/samples.cc

+void CommitWithBatchingDelay(google::cloud::spanner::Client client,
+			     google::cloud::spanner::Mutations mutations,
+			     absl::Duration batching_delay) {
+  google::cloud::spanner::CommitOptions options;


I think I've done this. This field is much like RequestPriorityOption, and I've pretty much copied it. I don't think we need to overload Commit() again, as the current versions all take an Options field. Please take a look and let me know if I've messed this up.

coryan · 2023-07-26T23:08:53Z

google/cloud/spanner/samples/samples.cc

+  Options ops;
+  ops.set<MaxBatchingDelayMsOption>(100);
+  auto commit_result = client.Commit(spanner::Mutations{
+      spanner::UpdateMutationBuilder("Albums",
+                                     {"SingerId", "AlbumId", "MarketingBudget"})
+          .EmplaceRow(1, 1, 100000)
+          .EmplaceRow(2, 2, 500000)
+          .Build()}, ops);


Suggested change

Options ops;

ops.set<MaxBatchingDelayMsOption>(100);

auto commit_result = client.Commit(spanner::Mutations{

spanner::UpdateMutationBuilder("Albums",

{"SingerId", "AlbumId", "MarketingBudget"})

.EmplaceRow(1, 1, 100000)

.EmplaceRow(2, 2, 500000)

.Build()}, ops);

auto commit_result = client.Commit(spanner::Mutations{

spanner::UpdateMutationBuilder("Albums",

{"SingerId", "AlbumId", "MarketingBudget"})

.EmplaceRow(1, 1, 100000)

.EmplaceRow(2, 2, 500000)

.Build()},

google::cloud::Options{}.set<spanner::MaxBatchDelayOption>(std::chrono::milliseconds(100));

coryan · 2023-07-26T23:10:12Z

google/cloud/spanner/options.h

+struct MaxBatchingDelayMsOption {
+  using Type = int;
+};
+


I do not think we would want to use int to represent time intervals:

Suggested change

struct MaxBatchingDelayMsOption {

using Type = int;

};

struct MaxBatchingDelayOption {

using Type = std::chrono::milliseconds;

};

coryan · 2023-07-26T23:11:36Z

google/cloud/spanner/commit_options.h

@@ -70,6 +70,16 @@ class CommitOptions {
    return request_priority_;
  }

+  CommitOptions& set_max_batching_delay_ms(


@devbww do we want to keep adding methods to this class? Can we support this new feature only via google::cloud::Options{}?

No to spanner::CommitOptions. Yes, to google::cloud::Options.

devbww · 2023-07-26T23:37:32Z

/gcbrun

coryan · 2023-07-27T17:39:02Z

FWIW, most builds are useless in this case. Except for this:

https://github.com/googleapis/google-cloud-cpp/pull/11939/checks?check_run_id=15378864615

coryan · 2023-11-30T14:58:42Z

This PR is looking really stale. What is the next step here?

devbww · 2023-11-30T21:45:58Z

This PR is looking really stale. What is the next step here?

CommitRequest.max_batching_delay is still unpublished. Indeed, the internal version now carries a "This field will be removed" warning, seemingly in favor of a new (also unpublished) max_commit_delay field.

So, at best this seems obsolete, and should be closed.

coryan · 2023-11-30T22:43:48Z

Closing for now. It can be reopened if needed or the same branch can be used to push a new PR.

nginsberg-google added 5 commits May 7, 2023 14:39

Add batching delay to commit request options.

ff7e0fc

add change to connection_impl.cc

6211d89

Add code sample

f94e59e

Update

ac533c7

builds

0521188

nginsberg-google requested a review from a team as a code owner June 21, 2023 23:16

nginsberg-google changed the title ~~Add max_batching_delay option to CommitRequest.~~ feat: Add max_batching_delay option to CommitRequest. Jun 21, 2023

coryan reviewed Jun 21, 2023

View reviewed changes

devbww reviewed Jun 21, 2023

View reviewed changes

Merge branch 'googleapis:main' into batching

2dac4b8

nginsberg-google had a problem deploying to external July 26, 2023 21:59 — with GitHub Actions Error

Reviewer comments

14940b6

nginsberg-google had a problem deploying to external July 26, 2023 22:49 — with GitHub Actions Error

Remove unneeded include

9bd2e56

nginsberg-google had a problem deploying to external July 26, 2023 22:50 — with GitHub Actions Failure

nginsberg-google commented Jul 26, 2023

View reviewed changes

coryan suggested changes Jul 26, 2023

View reviewed changes

coryan closed this Nov 30, 2023

feat: Add max_batching_delay option to CommitRequest. #11939

feat: Add max_batching_delay option to CommitRequest. #11939

Conversation

nginsberg-google commented Jun 21, 2023 • edited by coryan Loading

snippet-bot bot commented Jun 21, 2023 • edited Loading

devbww commented Jun 21, 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

Footnotes

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

coryan commented Jul 21, 2023

nginsberg-google commented Jul 26, 2023

nginsberg-google left a 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

devbww commented Jul 26, 2023

coryan commented Jul 27, 2023

coryan commented Nov 30, 2023

devbww commented Nov 30, 2023

coryan commented Nov 30, 2023

nginsberg-google commented Jun 21, 2023 •

edited by coryan

Loading

snippet-bot bot commented Jun 21, 2023 •

edited

Loading