impl(bigtable): add MutateRowsLimiter
#13079
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #13079 +/- ##
==========================================
- Coverage 92.99% 92.99% -0.01%
==========================================
Files 2131 2134 +3
Lines 185409 185559 +150
==========================================
+ Hits 172423 172560 +137
- Misses 12986 12999 +13
☔ View full report in Codecov by Sentry. |
| /// A Bigtable-specific wrapper over the more generic `RateLimiter` | ||
| class MutateRowsLimiter { | ||
| public: | ||
| virtual void acquire() = 0; |
There was a problem hiding this comment.
Capitalized on the opportunity
| std::chrono::duration<Rep1, Period1> initial_period, | ||
| std::chrono::duration<Rep2, Period2> min_period, | ||
| std::chrono::duration<Rep3, Period3> max_period, double min_factor, | ||
| double max_factor) |
There was a problem hiding this comment.
There was a problem hiding this comment.
TIL... do I need to change the variable to max_factor_tm ?
| public: | ||
| virtual void acquire() = 0; | ||
| virtual void Update( | ||
| google::bigtable::v2::MutateRowsResponse const& response) = 0; |
There was a problem hiding this comment.
Should this whole thing be divorced from MutateRows() by taking a google::bigtable::v2::RateLimitInfo instead, just making it bigtable_internal::RateLimiter? RateLimitInfo might be used in others calls in the future. (I guess the mutate-rows Update() call would become conditional.)
There was a problem hiding this comment.
Should this whole thing be divorced from
MutateRows()... RateLimitInfo might be used in others calls in the future.
This is a good question. RateLimitInfo is defined inside MutateRowsResponse though: https://github.com/googleapis/googleapis/blob/92a6b0fc959905f68e567dd60f766167772594bd/google/bigtable/v2/bigtable.proto#L513
The Bigtable team mentioned that the ReadRows API could be worthy of throttling. But I don't think they plan to add that. This feature is really aimed at heavy write jobs.
Moreover, the flags to enable things are called mutate_rows_rate_limit[0-9]*
(I guess the mutate-rows Update() call would become conditional.)
Yeah. This was my main motivation, although it's not a big deal.
There was a problem hiding this comment.
RateLimitInfois defined insideMutateRowsResponsethough
That's not what I see, hence the suggestion.
Moreover, the flags to enable things are called
mutate_rows_rate_limit[0-9]*
I don't think that's indicative of anything other than they are the flags for MutateRows(). If RateLimitInfo was used elsewhere, presumably that might get its own flags.
(I guess the mutate-rows
Update()call would become conditional.)Yeah. This was my main motivation, although it's not a big deal.
I agree that it's not a big deal, and therefore not a good main motivation. That is, it seems a shame to specialize the interface for that.
There was a problem hiding this comment.
That's not what I see, hence the suggestion.
D'oh. I can't read.
Still this class is internal. We can easily change it later.
There was a problem hiding this comment.
We can easily change it later.
True enough, although I think we suffer a little from we-can-change-it-later-itis.
There was a problem hiding this comment.
I don't disagree... but observation:
It will take as much effort to change it now as it will to change it later... and we might not need to change it later.
There was a problem hiding this comment.
We run into trouble when we do this a bunch, and then we need to change N of them at the same time. 🫨
| public: | ||
| virtual void acquire() = 0; | ||
| virtual void Update( | ||
| google::bigtable::v2::MutateRowsResponse const& response) = 0; |
There was a problem hiding this comment.
We can easily change it later.
True enough, although I think we suffer a little from we-can-change-it-later-itis.
Part of the work for #12959
Add a wrapper over
RateLimiterwhich knows how to update the limiter's rate as decreed by the server.I do not plan to expose any of
initial_period,min_period,max_period,min_factor, ormax_factor. But I want to abstract them so our tests do not break if we tweak the defaults.Also the reviewer may notice that there is both a sync and async version of
Table::BulkApply()(akaBigtableStub::MutateRows). But theMutateRowsLimitercan only sleep in band.The plan is to complete throttling for the synchronous API then worry about how it looks with the async API after.
This change is