feat: add rate limiter class by thebrianchen · Pull Request #1021 · googleapis/nodejs-firestore

thebrianchen · 2020-04-15T18:35:27Z

Adding a rate limiter that implements token bucket that calculates if a request containing a number of writes can be made under a rate limit as well as the time required before a request can be made while staying under the limit.

It's a little more verbose than I wanted, but having a separate rate limiter has a few benefits for BulkWriter:

We would not have to log sendTimes for each batch
We would not have to manually calculate the number of writes made in the past second each time we want to determine whether a batch can be sent.
Simplifies calculation for the required delay before the next batch can be sent.

Implementation notes:

I used a currentTime parameter to make testing the limiter easier, since it's based on Timestamps. I was hoping to get some feedback/suggestions on how to test future Timestamps without hurting readability of the class too much.
It is possible to factor in the next multiplier in calculating the required delay (ex: a request that would currently exceed the maximum capacity would be possible after the next capacity increase), but I don't think it's worth adding.

schmidt-sebastian

This is pretty nice! I wonder though if we are need to deduct tokens even if they are not used. What do you think?

schmidt-sebastian · 2020-04-15T21:53:46Z

+   * @private
+   */
+  // Visible for testing.
+  _tryMakeRequest(numOperations: number, currentTime: Timestamp): boolean {


Can you combine tryMakeRequest and _tryMakeRequest? You can assign a default value to currentTime (and name it something like requestTime to make it more suited for a public API).

I had that originally, but felt uncomfortable with the idea that you could break the limiter by passing in arbitrary timestamps far in the future. If someone specified a timestamp far in the future, the limiter would not refill any tokens until that time has been reached. It seems fragile to expose a parameter that would break the limiter, but if we're the only ones using the class, is it ok?

Renamed to requestTimeMillis.

schmidt-sebastian · 2020-04-15T21:56:43Z

+   * @private
+   */
+  private refillTokens(currentTime = Timestamp.now()): void {
+    if (currentTime.toMillis() > this.lastRefillTime.toMillis()) {


If you used Date object or even just epoch milliseconds your code will be much more portable and can be used outside of the FIrestore client if need be.

Switched to using Date and getTime().

schmidt-sebastian · 2020-04-15T22:01:33Z

+ *
+ * @private
+ */
+export class RateLimiter {


Do we have to add some code to make tokens expire even if they are not used? I would suspect that if you issue 500 writes per second, then sit idle for a second, you can only issue 500 writes and not 1000.

Although we are not making tokens expire, we are already limiting the maximum number of tokens that can be accrued. When we refill the tokens, we call Math.min() on the capacity and the number of added tokens to prevent the scenario you mentioned above from happening.

I also added another test case to cover this as well.

thebrianchen

thanks again ⏲️ ⌚ ⌛ ⏳

I wonder though if we are need to deduct tokens even if they are not used. What do you think?

See the comment I left, but I think we are already doing that in the code by limiting the number of tokens that can be accrued.

thebrianchen · 2020-04-16T00:41:39Z

+   * @private
+   */
+  // Visible for testing.
+  _tryMakeRequest(numOperations: number, currentTime: Timestamp): boolean {


I had that originally, but felt uncomfortable with the idea that you could break the limiter by passing in arbitrary timestamps far in the future. If someone specified a timestamp far in the future, the limiter would not refill any tokens until that time has been reached. It seems fragile to expose a parameter that would break the limiter, but if we're the only ones using the class, is it ok?

thebrianchen · 2020-04-16T01:25:32Z

+ *
+ * @private
+ */
+export class RateLimiter {


Although we are not making tokens expire, we are already limiting the maximum number of tokens that can be accrued. When we refill the tokens, we call Math.min() on the capacity and the number of added tokens to prevent the scenario you mentioned above from happening.

I also added another test case to cover this as well.

thebrianchen · 2020-04-16T01:52:09Z

+   * @private
+   */
+  private refillTokens(currentTime = Timestamp.now()): void {
+    if (currentTime.toMillis() > this.lastRefillTime.toMillis()) {


Switched to using Date and getTime().

schmidt-sebastian

Thanks for addressing the feedback!

I would like to push you once more to rename currentTimeMillis to requestTimeMillis. If you do that, then suddenly your testing only code becomes part of the public API and exposing it seems much more natural. This is optional if you disagree.

schmidt-sebastian · 2020-04-16T19:08:14Z

+   * @private
+   */
+  private refillTokens(currentTimeMillis = Date.now()): void {
+    if (currentTimeMillis > this.lastRefillTimeMillis) {


We might want to make the behavior of this class more predictable and reject timestamps that go back in time.

Added behavior to reject timestamps that are before the last refill time. Also added test for this.

thebrianchen

thanks sebastian!

thebrianchen · 2020-04-16T21:43:31Z

+   * @private
+   */
+  private refillTokens(currentTimeMillis = Date.now()): void {
+    if (currentTimeMillis > this.lastRefillTimeMillis) {


Added behavior to reject timestamps that are before the last refill time. Also added test for this.

Brian Chen added 2 commits April 15, 2020 11:19

add rate limiting

8c1430a

add internal methods

554172a

thebrianchen self-assigned this Apr 15, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 15, 2020

thebrianchen changed the title ~~Add RateLimiter~~ feat: add rate limiter class Apr 15, 2020

thebrianchen requested a review from schmidt-sebastian April 15, 2020 20:24

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Apr 15, 2020

schmidt-sebastian reviewed Apr 15, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Apr 15, 2020

thebrianchen commented Apr 16, 2020

View reviewed changes

change to use date

60c187a

thebrianchen requested a review from schmidt-sebastian April 16, 2020 01:59

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Apr 16, 2020

schmidt-sebastian approved these changes Apr 16, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Apr 16, 2020

resolve comments, add error for older timestamps

a008efd

thebrianchen commented Apr 16, 2020

View reviewed changes

thebrianchen merged commit 05773ba into bc/bulk-master Apr 16, 2020

thebrianchen deleted the bc/rate-limiter branch April 16, 2020 21:52

thebrianchen mentioned this pull request May 27, 2020

fix: add RateLimiter googleapis/java-firestore#230

Merged

Conversation

thebrianchen commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thebrianchen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thebrianchen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thebrianchen commented Apr 15, 2020 •

edited

Loading

thebrianchen left a comment •

edited

Loading