feat(bigtable): support BulkApply throttling #13124

dbolduc · 2023-11-13T22:57:48Z

Part of the work for #12959

Add an option to enable throttling of BulkApply requests. The option deserves scrutiny!!!!

API discussion

I decided not to put the option in an experimental namespace.

The option is only a bool. We could have exposed the other parameters like kInitialPeriod, k{Min,Max}Period, k{Min,Max}Factor.

Of those, I think only kInitialPeriod (or its inverse, InitialQps is useful. We have a default initial QPS of 20, but not all queries are created equally. Applications might know more about their queries than us. They might find this too low, and that it takes unnecessarily long to ramp up to Bigtable's limits.

An alternative would be the more general, but potentially more verbose:

struct BulkApplyThrottlingParams {
  bool enabled;
  // grow as needed. e.g. double initial_rate;
};

struct BulkApplyThrottlingOption {
  using Type = BulkApplyThrottlingParams;
};

Tracing

Defer tracing because I made an oopsie in the internal helper, and the PR is large enough already. FTR, enabling tracing for the integration test will eventually look like this:

Note how Table::BulkApply() takes ~50ms, matching our kInitialQps.

No, I do not have an explanation for why a few of the requests end up taking 6.5s 🤷

This change is

codecov · 2023-11-13T23:53:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (343c05b) 92.99% compared to head (b6aa428) 92.99%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13124   +/-   ##
=======================================
  Coverage   92.99%   92.99%           
=======================================
  Files        2137     2137           
  Lines      185782   185814   +32     
=======================================
+ Hits       172774   172806   +32     
  Misses      13008    13008

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

devbww · 2023-11-14T00:25:25Z

The option is only a bool. We could have exposed the other parameters like kInitialPeriod, k{Min,Max}Period, k{Min,Max}Factor.

Of those, I think only kInitialPeriod is useful.

Then how about...

struct BulkApplyThrottlingOption {
  using Type = std::chrono::duration<Rep, Period>;  // initial period
};

If the option is set, you get a ThrottlingMutateRowsLimiter configured with the initial period.

If the option is not set, you get a NoopMutateRowsLimiter.

dbolduc · 2023-11-14T04:46:27Z

The option is only a bool. We could have exposed the other parameters like kInitialPeriod, k{Min,Max}Period, k{Min,Max}Factor.
Of those, I think only kInitialPeriod is useful.

Let me amend my statement: Of those I think only kInitialPeriod could be useful.

Then how about...

struct BulkApplyThrottlingOption {
  using Type = std::chrono::duration<Rep, Period>;  // initial period
};

I do like your suggestion.

We cannot template the Type, right? I think you are just illustrating the idea. I think std::chrono::milliseconds would be fine for the duration type.

It would be nice if the application could easily set the option, and let the library give it a default. This could mean:

we document a suggested default
we use Type = absl::optional<std::chrono::milliseconds>
we say a value of duration::zero() means the library can pick the default. (I don't like this idea, because zero() can and should mean: start without any throttling).

I'll sit on it for a day. If anyone else reads this, please jump in with your opinions, however weakly held they may be.

coryan · 2023-11-14T13:24:05Z

I'll sit on it for a day. If anyone else reads this, please jump in with your opinions, however weakly held they may be.

I think this deserves a document. Should not be more than a couple of pages, maybe we can review in the team meeting.

dbolduc · 2023-11-15T23:07:37Z

We discussed this in the team meeting and decided not to expose the initial period. We also decided that bool options aren't exactly ON/OFF, we should use absl::monostate instead. We also decided to initially put this in an experimental namespace. We know of one internal Google team who wants to adopt this feature. We should get feedback from them on whether ON/OFF is sufficient before we decide to release the feature.

Googlers can see the discussion at: go/cloud-cxx:bulk-apply-throttling-api

devbww · 2023-11-16T00:00:47Z

google/cloud/bigtable/options.h

+ * https://cloud.google.com/bigtable/docs/routing#single-cluster
+ */
+struct BulkApplyThrottlingOption {
+  using Type = absl::monostate;


#include "absl/types/variant.h"

And did you consider absl::variant<absl::monostate>?

#include "absl/types/variant.h"

yep

And did you consider absl::variant<absl::monostate>?

Not really. I guess that lets us grow the variant to include a second type. Although if we end up with absl::variant<absl::monostate, std::chrono::microseconds>, we might have preferred absl::optional<std::chrono::microseconds>...

Hmm.. I changed it.

I guess that lets us grow the variant

Nevermind. We can't grow the variant. This code stops compiling if Type = absl::variant<absl::monostate, T>.

absl::variant<absl::monostate> v = options.get<BulkApplyThrottlingOption>();

@devbww I reverted the type to bool. I will wait for your OK before submitting the PR.

I'm not sure I'm claiming things will compile unchanged (although I think they could if someone didn't care about new variants), but rather that the migration path is clear. But, I can't predict the future (and this wouldn't be high on the list if I could!), so OK. Thanks for taking a look.

devbww · 2023-11-16T00:05:39Z

google/cloud/bigtable/tests/data_integration_test.cc

+                    TableTestEnvironment::instance_id(),
+                    TableTestEnvironment::table_id()));
+
+  // This test will take at least 100 queries / (20 QPS) = 5s.


Perhaps we should be able to inject a "sleeper" via the options.

Ack. We could also pass MockConnections to Make*Connection(Options const& options) via the options for better testing of those functions.

Although, at some point we are going to need an integration test for the real implementation.

OK, but if tests start sleeping for 5s here and there, we're only hurting ourselves.

Reduced from 100 -> 10 requests.

devbww · 2023-11-16T03:47:33Z

google/cloud/bigtable/tests/data_integration_test.cc

+                    TableTestEnvironment::instance_id(),
+                    TableTestEnvironment::table_id()));
+
+  // This test will take at least 100 queries / (20 QPS) = 5s.


OK, but if tests start sleeping for 5s here and there, we're only hurting ourselves.

devbww · 2023-11-16T03:48:01Z

google/cloud/bigtable/tests/data_integration_test.cc

+      MakeDataConnection(Options{}.set<experimental::BulkApplyThrottlingOption>(
+          absl::monostate{})),


Q. Would this test fail without the option?

No, but maybe something would break if we implemented the feature incorrectly?

The behavior of the feature is observable. We could time the test to make sure it takes at least the initial period * N requests. I am meh on adding that sort of check though.

The clock we would use in our test would be the same clock as is used in the limiter. So I do not think we would see time based errors/flakes.

Anecdotally from my own testing, the server takes a few requests before it sends any rate limiting information back. I don't want to rely on that behavior though. If the server tells us to speed up immediately, then the test may take < initial period * N requests, breaking the expectation.

maybe something would break if we implemented the feature incorrectly?

Maybe? I guess code coverage is a good thing, but we should probably try not to leave an impression that we're testing any behavior.

I added a comment to leave no impression that we're testing any behavior.

dbolduc temporarily deployed to internal November 13, 2023 22:57 — with GitHub Actions Inactive

product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Nov 13, 2023

dbolduc marked this pull request as ready for review November 13, 2023 23:31

dbolduc requested a review from a team as a code owner November 13, 2023 23:31

dbolduc marked this pull request as draft November 15, 2023 22:02

dbolduc added 2 commits November 15, 2023 17:32

feat(bigtable): support BulkApply throttling

0575df5

update option, per discussion

5fb074b

dbolduc force-pushed the bigtable-make-limiter branch from dbbf871 to 5fb074b Compare November 15, 2023 23:33

dbolduc temporarily deployed to internal November 15, 2023 23:33 — with GitHub Actions Inactive

fix includes

b926fa8

devbww reviewed Nov 16, 2023

View reviewed changes

dbolduc temporarily deployed to internal November 16, 2023 00:07 — with GitHub Actions Inactive

address review comments

a078025

dbolduc temporarily deployed to internal November 16, 2023 00:21 — with GitHub Actions Inactive

dbolduc marked this pull request as ready for review November 16, 2023 01:17

devbww approved these changes Nov 16, 2023

View reviewed changes

back to bool

0459ada

dbolduc temporarily deployed to internal November 16, 2023 12:36 — with GitHub Actions Inactive

devbww approved these changes Nov 16, 2023

View reviewed changes

address review comments

b6aa428

dbolduc temporarily deployed to internal November 16, 2023 19:06 — with GitHub Actions Inactive

dbolduc enabled auto-merge (squash) November 16, 2023 19:08

dbolduc merged commit 294d92e into googleapis:main Nov 16, 2023
59 checks passed

dbolduc deleted the bigtable-make-limiter branch November 16, 2023 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bigtable): support BulkApply throttling #13124

feat(bigtable): support BulkApply throttling #13124

dbolduc commented Nov 13, 2023 •

edited

codecov bot commented Nov 13, 2023 •

edited

devbww commented Nov 14, 2023

dbolduc commented Nov 14, 2023

coryan commented Nov 14, 2023

dbolduc commented Nov 15, 2023 •

edited

devbww Nov 16, 2023

dbolduc Nov 16, 2023

dbolduc Nov 16, 2023

dbolduc Nov 16, 2023

devbww Nov 16, 2023

devbww Nov 16, 2023

dbolduc Nov 16, 2023

devbww Nov 16, 2023

dbolduc Nov 16, 2023

devbww Nov 16, 2023

devbww Nov 16, 2023

dbolduc Nov 16, 2023

devbww Nov 16, 2023

dbolduc Nov 16, 2023

		MakeDataConnection(Options{}.set<experimental::BulkApplyThrottlingOption>(
		absl::monostate{})),

feat(bigtable): support BulkApply throttling #13124

feat(bigtable): support BulkApply throttling #13124

Conversation

dbolduc commented Nov 13, 2023 • edited

API discussion

Tracing

codecov bot commented Nov 13, 2023 • edited

Codecov Report

devbww commented Nov 14, 2023

dbolduc commented Nov 14, 2023

coryan commented Nov 14, 2023

dbolduc commented Nov 15, 2023 • edited

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

dbolduc commented Nov 13, 2023 •

edited

codecov bot commented Nov 13, 2023 •

edited

dbolduc commented Nov 15, 2023 •

edited