fix: add RateLimiter #230

thebrianchen · 2020-05-27T14:39:54Z

Porting from node.

codecov · 2020-05-27T14:44:21Z

Codecov Report

Merging #230 into master will increase coverage by 0.06%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master     #230      +/-   ##
============================================
+ Coverage     72.94%   73.01%   +0.06%     
- Complexity     1015     1035      +20     
============================================
  Files            63       64       +1     
  Lines          5407     5443      +36     
  Branches        617      618       +1     
============================================
+ Hits           3944     3974      +30     
- Misses         1266     1271       +5     
- Partials        197      198       +1

Impacted Files	Coverage Δ	Complexity Δ
...n/java/com/google/cloud/firestore/RateLimiter.java	`83.33% <83.33%> (ø)`	`9.00 <9.00> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a39775f...b0ace80. Read the comment docs.

schmidt-sebastian

Nice! This is pretty much good to go, unless we can just the RateLimiter in Google Common, in which case we can close this PR altogether. Thank you!

schmidt-sebastian · 2020-05-27T23:42:00Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

+ * <p>RateLimiter can also implement a gradually increasing rate limit. This is used to enforce the
+ * 500/50/5 rule (https://cloud.google.com/datastore/docs/best-practices#ramping_up_traffic).
+ */
+public class RateLimiter {


There is already a RateLimiter in our classpath. Did you take a look if we can use it?

https://guava.dev/releases/19.0/api/docs/index.html?com/google/common/util/concurrent/RateLimiter.html

It supports a warm up period, but I am not sure if it matches our exact needs.

The main limitation for the provided one is that the rate does not ramp up, so we would have to add logic to adjust the rate based on the start time. It also doesn't return the seconds required for the next available call and blocks instead. I'm not sure if that would affect the implementation in Java, but given that this is ~100 lines, I'm more in favor of just copying it over from node to make porting to other languages easier.

I was hoping the ramp up provided by this Google provided class would support our own network traffic guidance, but I also have my doubts. FWIW, I do think that at least one of the libraries we port to might have an adequate rate limiting algorithm already, and showing how to use one via an example would also have been helpful.

Unrelated: This class should not be public. I assume we can just make this package-private.

changed to package-private. Thanks for the explanation -- I had not considered the value of an example that uses existing rate limiting algorithms.

schmidt-sebastian · 2020-05-27T23:43:17Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

+ * before a request can be made.
+ *
+ * <p>RateLimiter can also implement a gradually increasing rate limit. This is used to enforce the
+ * 500/50/5 rule (https://cloud.google.com/datastore/docs/best-practices#ramping_up_traffic).


Use @link JavaDoc here.

I think it's @see: https://stackoverflow.com/questions/1082050/linking-to-an-external-url-in-javadoc

I would drop the quotes and maybe make this an <a href=> link like in those examples (it feels like overkill though).

added href wth the text "Ramping up traffic"

schmidt-sebastian · 2020-05-27T23:44:15Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

+  final int initialCapacity;
+  final double multiplier;
+  final int multiplierMillis;
+  final long startTimeMillis;


Can these be private?

schmidt-sebastian · 2020-05-27T23:46:29Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

+    if (requestTimeMillis >= lastRefillTimeMillis) {
+      long elapsedTime = requestTimeMillis - lastRefillTimeMillis;
+      int capacity = calculateCapacity(requestTimeMillis);
+      int tokensToAdd = (int) Math.floor((elapsedTime * capacity) / 1000);


You can just cast to "int". It does Math.floor() for you.

schmidt-sebastian · 2020-05-27T23:47:46Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

+        lastRefillTimeMillis = requestTimeMillis;
+      }
+    } else {
+      throw new IllegalArgumentException(


Optional: You Preconditions.checkArgument as first statement of this method.

Is there any benefit to using Preconditions.checkArgument instead of throwing it here? Otherwise keeping it as is would mirror node.

Benefits:

It is more Java idiomatic

It removes the indent

It will probably remove two lines from this method

With that being said, totally optional to change this.

changed. thanks for writing it out! I did not realize that checkArgument pattern was a Java idiom.

schmidt-sebastian · 2020-05-27T23:48:14Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

+  public int calculateCapacity(long requestTimeMillis) {
+    long millisElapsed = requestTimeMillis - startTimeMillis;
+    int operationsPerSecond =
+        (int)


Same comment regarding Math.floor + cast.

schmidt-sebastian · 2020-05-28T16:46:23Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

+ * <p>RateLimiter can also implement a gradually increasing rate limit. This is used to enforce the
+ * 500/50/5 rule (https://cloud.google.com/datastore/docs/best-practices#ramping_up_traffic).
+ */
+public class RateLimiter {


I was hoping the ramp up provided by this Google provided class would support our own network traffic guidance, but I also have my doubts. FWIW, I do think that at least one of the libraries we port to might have an adequate rate limiting algorithm already, and showing how to use one via an example would also have been helpful.

schmidt-sebastian · 2020-05-28T16:48:04Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

+        lastRefillTimeMillis = requestTimeMillis;
+      }
+    } else {
+      throw new IllegalArgumentException(


Benefits:

It is more Java idiomatic

It removes the indent

It will probably remove two lines from this method

With that being said, totally optional to change this.

schmidt-sebastian · 2020-05-28T16:51:37Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

+ * before a request can be made.
+ *
+ * <p>RateLimiter can also implement a gradually increasing rate limit. This is used to enforce the
+ * 500/50/5 rule (https://cloud.google.com/datastore/docs/best-practices#ramping_up_traffic).


I would drop the quotes and maybe make this an <a href=> link like in those examples (it feels like overkill though).

schmidt-sebastian · 2020-05-28T16:52:57Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/RateLimiter.java

+ * <p>RateLimiter can also implement a gradually increasing rate limit. This is used to enforce the
+ * 500/50/5 rule (https://cloud.google.com/datastore/docs/best-practices#ramping_up_traffic).
+ */
+public class RateLimiter {


Unrelated: This class should not be public. I assume we can just make this package-private.

fix: add RateLimiter

0913cb2

thebrianchen self-assigned this May 27, 2020

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

s/date/time

b92b58c

thebrianchen requested a review from schmidt-sebastian May 27, 2020 20:27

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen May 27, 2020

schmidt-sebastian reviewed May 27, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian May 27, 2020

resolve comments

785488b

thebrianchen requested a review from schmidt-sebastian May 28, 2020 01:15

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen May 28, 2020

schmidt-sebastian approved these changes May 28, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian May 28, 2020

add href + use checkArgument

b0ace80

thebrianchen merged commit 47d4a11 into master May 28, 2020

thebrianchen deleted the bc/bulk-limit branch May 28, 2020 17:49

thebrianchen restored the bc/bulk-limit branch May 28, 2020 17:49

thebrianchen deleted the bc/bulk-limit branch May 28, 2020 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add RateLimiter #230

fix: add RateLimiter #230

thebrianchen commented May 27, 2020

codecov bot commented May 27, 2020 •

edited

Loading

schmidt-sebastian left a comment

schmidt-sebastian May 27, 2020

thebrianchen May 28, 2020

schmidt-sebastian May 28, 2020

schmidt-sebastian May 28, 2020

thebrianchen May 28, 2020

schmidt-sebastian May 27, 2020

thebrianchen May 28, 2020

schmidt-sebastian May 28, 2020

thebrianchen May 28, 2020

schmidt-sebastian May 27, 2020

thebrianchen May 28, 2020

schmidt-sebastian May 27, 2020

thebrianchen May 28, 2020

schmidt-sebastian May 27, 2020

thebrianchen May 28, 2020

schmidt-sebastian May 28, 2020

thebrianchen May 28, 2020

schmidt-sebastian May 27, 2020

thebrianchen May 28, 2020

schmidt-sebastian May 28, 2020

schmidt-sebastian May 28, 2020

schmidt-sebastian May 28, 2020

schmidt-sebastian May 28, 2020

fix: add RateLimiter #230

fix: add RateLimiter #230

Conversation

thebrianchen commented May 27, 2020

codecov bot commented May 27, 2020 • edited Loading

Codecov Report

schmidt-sebastian 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

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

codecov bot commented May 27, 2020 •

edited

Loading