impl(bigtable): add a generic RateLimiter #13060

dbolduc · 2023-11-06T23:06:06Z

Part of the work for #12959

Add a generic interface for rate limiting. Put it in bigtable, because that is the only place it is needed for now. We can always move it to common if the need arises.

We do not include any Bigtable specific logic (e.g. how to update the rate given a response) in this interface. We will add another class for that later.

This change is

codecov · 2023-11-06T23:35:27Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4028827) 92.99% compared to head (094d05b) 92.99%.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #13060    +/-   ##
========================================
  Coverage   92.99%   92.99%            
========================================
  Files        2128     2131     +3     
  Lines      185258   185383   +125     
========================================
+ Hits       172272   172397   +125     
  Misses      12986    12986

Files	Coverage Δ
google/cloud/bigtable/internal/rate_limiter.cc	`100.00% <100.00%> (ø)`
google/cloud/bigtable/internal/rate_limiter.h	`100.00% <100.00%> (ø)`
...oogle/cloud/bigtable/internal/rate_limiter_test.cc	`99.01% <99.01%> (ø)`

... and 6 files with indirect coverage changes

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

google/cloud/bigtable/internal/rate_limiter.h

google/cloud/bigtable/internal/rate_limiter.cc

google/cloud/bigtable/internal/rate_limiter.h

devbww · 2023-11-07T03:35:03Z

google/cloud/bigtable/internal/rate_limiter.h

+  // increase with time.
+  using Clock = ::google::cloud::internal::SteadyClock;
+
+  explicit RateLimiter(std::shared_ptr<Clock> clock, double rate,


Why shared_ptr?

I was copying

google-cloud-cpp/google/cloud/spanner/internal/session.h

Line 40 in 09cfff7

std::shared_ptr<Clock> clock = std::make_shared<Clock>())

I think part of the reason is that our FakeClock has a std::mutex in it.

google-cloud-cpp/google/cloud/testing_util/fake_clock.h

Line 59 in 09cfff7

mutable std::mutex mu_;

I think the issue there is the way the Session constructor defaults clock. You don't have that problem, and so could take Clock* clock with a lifetime caveat. Tests should all have a single clock that lasts the lifetime of the test, and non-tests should have a single, global-extent clock, period. No need for shared ownership in either case.

Hm I think I would rather avoid the caveats and leave it as a shared_ptr.

google/cloud/bigtable/internal/rate_limiter.h

google/cloud/bigtable/internal/rate_limiter.cc

devbww · 2023-11-07T03:56:36Z

google/cloud/bigtable/internal/rate_limiter.cc

+
+using Clock = RateLimiter::Clock;
+
+/**


why?

I know /** is a doxygen / javadoc thing. I find it simpler to write comments the same way for public and private APIs.

Why? Because it is better to be intentional than to act by default. And, it is better to make things easier to read than to write. But, your choice.

I think we use /** more often than /* for internal documentation. I am not going to change these.

it is better to make things easier to read than to write

Agreed. Don't think it applies here.

google/cloud/bigtable/internal/rate_limiter.cc

google/cloud/bigtable/internal/rate_limiter.h

coryan · 2023-11-07T13:35:28Z

google/cloud/bigtable/internal/rate_limiter.h

+/**
+ * A threadsafe interface for rate limiting.
+ *
+ * The caller tells the `RateLimiter` how many permits it wants to acquire. The


I would start with something like:

The caller needs to acquire a "permit" to perform the operation under rate limits. This class limits the number of permits issued per period of time, effectively limiting the operation rate. The caller may acquire more than one permit at a time, if it needs to perform a burst of the operation under rate limit. More permits become available as time passes, with some maximum to limit the size of bursts.

I think "token" or "credit" is a more common name than "permit", but is not like any of the three is universally accepted.

You may want to quote: https://en.wikipedia.org/wiki/Flow_control_(data)#Open-loop_flow_control or whatever ATM networks called their flow control mechanism.

I would start with something like:

Thanks, it is what I was trying to say, but clearer.

or whatever ATM networks called their flow control mechanism.

Why do you know these things?

coryan · 2023-11-07T13:37:30Z

google/cloud/bigtable/internal/rate_limiter.h

+  /**
+   * Set the period.
+   *
+   * Note that the current next_ has already been calculated. This new rate will
+   * not apply to it. The new rate will apply to every `acquire()` after next.
+   */
+  void set_period(Clock::duration period) {
+    if (period < Clock::duration::zero()) {
+      GCP_LOG(FATAL) << "RateLimiter period must be >= 0.";
+    }
+    std::lock_guard<std::mutex> lk(mu_);
+    period_ = period;
+  }


This seems like a dangerous function. Do you really need it? If not, consider it removing it.

Consider a different name, maybe set_future_period().

Do you really need it?

Yeah, I really need it. I need to be able to change the rate on the fly as the server tells me to speed up / slow down.

Consider a different name

meh

coryan · 2023-11-07T13:37:33Z

google/cloud/bigtable/internal/rate_limiter_test.cc

@@ -0,0 +1,197 @@
+// Copyright 2023 Google Inc.


Fixed in the three files.

coryan · 2023-11-07T13:43:37Z

google/cloud/bigtable/internal/rate_limiter.cc

+    // The request can go through immediately. But first, we need to update the
+    // time the next permit can be given out.
+
+    // Update the stored permits


Some comments have periods, others don't.

Fixed by only having one comment.

coryan · 2023-11-07T13:46:13Z

google/cloud/bigtable/internal/rate_limiter.cc

+RateLimiter::Clock::duration RateLimiter::acquire(std::int64_t permits) {
+  auto const now = clock_->Now();
+  std::lock_guard<std::mutex> lk(mu_);
+  if (next_ <= now) {


Suggested change

if (next_ <= now) {

if (next_ > now) {

auto wait = next_ - now;

next_ += permits * period_;

return wait;

}

moot now, but if (next > now) was the simpler of the branches. Thanks.

coryan · 2023-11-07T13:48:03Z

google/cloud/bigtable/internal/rate_limiter.h

+   * operation. For example, instead of acquiring one permit per request, you
+   * might choose to acquire one permit per repeated field in a request.
+   */
+  Clock::duration acquire(std::int64_t permits);


Maybe this should return the actual number of permits granted and the sleep time? Right now it assumes the caller is using the class correctly, without opportunity for early wake ups from a sleep.

Maybe this should return the actual number of permits granted and the sleep time?

I do not know what you mean. This class always grants all of the permits it is asked for.

I think you are saying: return how many of the requested permits can be granted immediately?

I don't want to do that. If the caller can split up the work, they should split up the calls to acquire().

Right now it assumes the caller is using the class correctly

Yeah.

devbww · 2023-11-07T13:14:21Z

google/cloud/bigtable/internal/rate_limiter.h

+        stored_permits_(max_stored_permits),
+        max_stored_permits_(max_stored_permits) {
+    if (period_ < Clock::duration::zero()) {
+      GCP_LOG(FATAL) << "RateLimiter period must be > 0.";


s/>/>=/

[You might also consider just using the absolute value.]

Let's go with absolute value.

devbww · 2023-11-07T13:16:14Z

google/cloud/bigtable/internal/rate_limiter.h

+/**
+ * A threadsafe interface for rate limiting.
+ *
+ * The caller tells the `RateLimiter` how many permits it wants to acquire. The


Now consider s/permits/cycles/ (which I like) or s/permits/tokens/ (which may be more familiar).

We are going with "tokens".

devbww · 2023-11-07T13:26:51Z

google/cloud/bigtable/internal/rate_limiter.cc

+
+using Clock = RateLimiter::Clock;
+
+/**


Why? Because it is better to be intentional than to act by default. And, it is better to make things easier to read than to write. But, your choice.

google/cloud/bigtable/internal/rate_limiter.cc

google/cloud/bigtable/internal/rate_limiter.h

devbww · 2023-11-07T13:39:07Z

google/cloud/bigtable/internal/rate_limiter.h

+   * Note that the current next_ has already been calculated. This new rate will
+   * not apply to it. The new rate will apply to every `acquire()` after next.
+   */
+  void set_period(Clock::duration period) {


Why not accept the same period types as the constructor? (Or, alternatively, why not restrict the constructor type to Clock::duration? And if you do that, move this implementation to the .cc file.)

Fixed, thanks.

why not restrict the constructor type to Clock::duration?

I'd rather duration_cast than make any assumptions about Clock::duration

I'd rather do the duration_cast in this class, than have the caller do it.

google/cloud/bigtable/internal/rate_limiter.cc

devbww · 2023-11-07T14:11:32Z

google/cloud/bigtable/internal/rate_limiter.cc

+RateLimiter::Clock::duration RateLimiter::acquire(std::int64_t permits) {
+  auto const now = clock_->Now();
+  std::lock_guard<std::mutex> lk(mu_);
+  if (next_ <= now) {


It seems strange that these cases are so different, when you'd like to think that next_ == now could go either way and have the same effect.

Consider accounting for stored_permits_ within next_ (e.g., (stored_permits_ == 1, next_ == t) => (next_ == t - period_). Then, perhaps, all these distinctions disappear.

I am frankly embarrassed by how much simpler the code is. Thanks for the help.

devbww · 2023-11-07T14:20:29Z

google/cloud/bigtable/internal/rate_limiter.h

+  // increase with time.
+  using Clock = ::google::cloud::internal::SteadyClock;
+
+  explicit RateLimiter(std::shared_ptr<Clock> clock, double rate,


I think the issue there is the way the Session constructor defaults clock. You don't have that problem, and so could take Clock* clock with a lifetime caveat. Tests should all have a single clock that lasts the lifetime of the test, and non-tests should have a single, global-extent clock, period. No need for shared ownership in either case.

google/cloud/bigtable/internal/rate_limiter.cc

google/cloud/bigtable/internal/rate_limiter.h

google/cloud/bigtable/internal/rate_limiter_test.cc

devbww · 2023-11-07T17:54:25Z

google/cloud/bigtable/internal/rate_limiter.h

+        next_(clock_->Now() - max_stored_tokens * period_),
+        max_stored_tokens_(max_stored_tokens) {}


I entered this comment before, but I don't see it now, so I must have done something wrong. Sorry if it shows up twice.

It looks like you could do some strength reduction by storing max_stored_tokens * period instead of max_stored_tokens_ by itself.

Done.

Changed the interface to accept the "smoothing interval". This seems nicer than storing some volume that must be updated every time we change the rate. (I am open to other names).

Changed the implementation to start with an empty bank. (This made the tests nicer. I don't think it's a big deal either way).

google/cloud/bigtable/internal/rate_limiter.cc

google/cloud/bigtable/internal/rate_limiter.h

google/cloud/bigtable/internal/rate_limiter_test.cc

dbolduc temporarily deployed to internal November 6, 2023 23:06 — with GitHub Actions Inactive

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

dbolduc temporarily deployed to internal November 6, 2023 23:47 — with GitHub Actions Inactive

alevenberg reviewed Nov 7, 2023

View reviewed changes

google/cloud/bigtable/internal/rate_limiter.h Outdated Show resolved Hide resolved

dbolduc marked this pull request as ready for review November 7, 2023 01:09

dbolduc requested a review from a team as a code owner November 7, 2023 01:09

alevenberg reviewed Nov 7, 2023

View reviewed changes

devbww reviewed Nov 7, 2023

View reviewed changes

dbolduc temporarily deployed to internal November 7, 2023 08:29 — with GitHub Actions Inactive

dbolduc temporarily deployed to internal November 7, 2023 12:45 — with GitHub Actions Inactive

coryan reviewed Nov 7, 2023

View reviewed changes

devbww reviewed Nov 7, 2023

View reviewed changes

dbolduc temporarily deployed to internal November 7, 2023 16:56 — with GitHub Actions Inactive

devbww reviewed Nov 7, 2023

View reviewed changes

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

devbww approved these changes Nov 7, 2023

View reviewed changes

google/cloud/bigtable/internal/rate_limiter.cc Show resolved Hide resolved

google/cloud/bigtable/internal/rate_limiter.h Outdated Show resolved Hide resolved

google/cloud/bigtable/internal/rate_limiter_test.cc Outdated Show resolved Hide resolved

dbolduc added 7 commits November 8, 2023 10:31

impl(bigtable): add a generic RateLimiter

cd80357

fix types

5728d0e

address review comments

0ed2167

help compiler

efa8d11

address review comments

458b9c7

address review comments

5b242ce

address review comments

094d05b

dbolduc force-pushed the bigtable-rate-limiter branch from 56531f9 to 094d05b Compare November 8, 2023 16:31

dbolduc temporarily deployed to internal November 8, 2023 16:31 — with GitHub Actions Inactive

dbolduc merged commit c092cc9 into googleapis:main Nov 8, 2023

dbolduc deleted the bigtable-rate-limiter branch November 8, 2023 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl(bigtable): add a generic RateLimiter #13060

impl(bigtable): add a generic RateLimiter #13060

dbolduc commented Nov 6, 2023 •

edited by coryan

Loading

codecov bot commented Nov 6, 2023 •

edited

Loading

devbww Nov 7, 2023

dbolduc Nov 7, 2023

devbww Nov 7, 2023

dbolduc Nov 7, 2023

devbww Nov 7, 2023

dbolduc Nov 7, 2023

devbww Nov 7, 2023

dbolduc Nov 7, 2023

coryan Nov 7, 2023

dbolduc Nov 7, 2023

coryan Nov 7, 2023

dbolduc Nov 7, 2023

coryan Nov 7, 2023

dbolduc Nov 7, 2023

coryan Nov 7, 2023

dbolduc Nov 7, 2023

coryan Nov 7, 2023

dbolduc Nov 7, 2023

coryan Nov 7, 2023

dbolduc Nov 7, 2023

devbww Nov 7, 2023

dbolduc Nov 7, 2023

devbww Nov 7, 2023

dbolduc Nov 7, 2023

devbww Nov 7, 2023

devbww Nov 7, 2023

dbolduc Nov 7, 2023 •

edited

Loading

devbww Nov 7, 2023

dbolduc Nov 7, 2023

devbww Nov 7, 2023

devbww Nov 7, 2023

dbolduc Nov 7, 2023

-  if (next_ <= now) {
+  if (next_ > now) {
+    auto wait = next_ - now;
+    next_ += permits * period_;
+    return wait;
+  }

		next_(clock_->Now() - max_stored_tokens * period_),
		max_stored_tokens_(max_stored_tokens) {}

impl(bigtable): add a generic RateLimiter #13060

impl(bigtable): add a generic RateLimiter #13060

Conversation

dbolduc commented Nov 6, 2023 • edited by coryan Loading

codecov bot commented Nov 6, 2023 • edited Loading

Codecov Report

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

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 Nov 7, 2023 • edited Loading

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 6, 2023 •

edited by coryan

Loading

codecov bot commented Nov 6, 2023 •

edited

Loading

dbolduc Nov 7, 2023 •

edited

Loading