Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add RateLimiter #230

Merged
merged 4 commits into from May 28, 2020
Merged

fix: add RateLimiter #230

merged 4 commits into from May 28, 2020

Conversation

thebrianchen
Copy link

Porting from node.

@thebrianchen thebrianchen self-assigned this May 27, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 27, 2020
@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

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

Impacted file tree graph

@@             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.

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

* <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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* 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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use @link JavaDoc here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added href wth the text "Ramping up traffic"

final int initialCapacity;
final double multiplier;
final int multiplierMillis;
final long startTimeMillis;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these be private?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

lastRefillTimeMillis = requestTimeMillis;
}
} else {
throw new IllegalArgumentException(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public int calculateCapacity(long requestTimeMillis) {
long millisElapsed = requestTimeMillis - startTimeMillis;
int operationsPerSecond =
(int)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment regarding Math.floor + cast.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

* <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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lastRefillTimeMillis = requestTimeMillis;
}
} else {
throw new IllegalArgumentException(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* 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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* <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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thebrianchen thebrianchen merged commit 47d4a11 into master May 28, 2020
@thebrianchen thebrianchen deleted the bc/bulk-limit branch May 28, 2020 17:49
@thebrianchen thebrianchen restored the bc/bulk-limit branch May 28, 2020 17:49
@thebrianchen thebrianchen deleted the bc/bulk-limit branch May 28, 2020 17:49
gcf-merge-on-green bot pushed a commit that referenced this pull request Jun 4, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.34.0](https://www.github.com/googleapis/java-firestore/compare/v1.33.0...v1.34.0) (2020-05-29)


### Features

* add support for BigDecimal to CustomClassMapper ([#196](https://www.github.com/googleapis/java-firestore/issues/196)) ([a471f1e](https://www.github.com/googleapis/java-firestore/commit/a471f1eed1e555e95b3d9bcda31ce0277e35a14a))
* Create CODEOWNERS ([#207](https://www.github.com/googleapis/java-firestore/issues/207)) ([cd19eae](https://www.github.com/googleapis/java-firestore/commit/cd19eae68a4898a53c6c3cc8189eab30545a661d))


### Bug Fixes

* add RateLimiter ([#230](https://www.github.com/googleapis/java-firestore/issues/230)) ([47d4a11](https://www.github.com/googleapis/java-firestore/commit/47d4a11625d5888d6f31e494923853a08bb8af77))
* catch null Firestore in system tests ([#215](https://www.github.com/googleapis/java-firestore/issues/215)) ([2a4a7b5](https://www.github.com/googleapis/java-firestore/commit/2a4a7b50d40ff1c165e3d359d5f4eaf929f6ffbc))
* Fields used in whereIn should be equality filters ([#216](https://www.github.com/googleapis/java-firestore/issues/216)) ([4a62633](https://www.github.com/googleapis/java-firestore/commit/4a626333e5af0d70a4dc4853ed373dcf50ea0f4a))
* replace usages of transform proto with update_transform ([#213](https://www.github.com/googleapis/java-firestore/issues/213)) ([46a3c51](https://www.github.com/googleapis/java-firestore/commit/46a3c51386b57f20bd65c564e93181e9ce399e2b))
* support array of references for IN queries ([#211](https://www.github.com/googleapis/java-firestore/issues/211)) ([b376003](https://www.github.com/googleapis/java-firestore/commit/b3760032952529f148065928c3bf13ff73a34edd))


### Dependencies

* update core dependencies to v1.93.5 ([#229](https://www.github.com/googleapis/java-firestore/issues/229)) ([b078213](https://www.github.com/googleapis/java-firestore/commit/b078213209f3936cfe9c9e2cdea040c1262621d4))
* update dependency com.google.api:api-common to v1.9.1 ([#228](https://www.github.com/googleapis/java-firestore/issues/228)) ([7e4568d](https://www.github.com/googleapis/java-firestore/commit/7e4568d8b3f0fc6f591640ccc2d646eb2764e572))
* update dependency com.google.api.grpc:proto-google-common-protos to v1.18.0 ([#204](https://www.github.com/googleapis/java-firestore/issues/204)) ([1e05de4](https://www.github.com/googleapis/java-firestore/commit/1e05de4ecfde055a1c84c2f6dd338604b8580a61))
* update dependency com.google.cloud:google-cloud-conformance-tests to v0.0.10 ([#197](https://www.github.com/googleapis/java-firestore/issues/197)) ([69372af](https://www.github.com/googleapis/java-firestore/commit/69372af7253564691b291766e2bf4d80e9ecc770))
* update dependency com.google.guava:guava-bom to v29 ([#180](https://www.github.com/googleapis/java-firestore/issues/180)) ([3c204b4](https://www.github.com/googleapis/java-firestore/commit/3c204b42ddfbe435ac095368d1e695ed282280bd))
* update dependency io.grpc:grpc-bom to v1.29.0 ([#206](https://www.github.com/googleapis/java-firestore/issues/206)) ([5d8c50f](https://www.github.com/googleapis/java-firestore/commit/5d8c50f105649100abf4fa7a6882bb0469ccbf8f))
* update dependency org.threeten:threetenbp to v1.4.4 ([#194](https://www.github.com/googleapis/java-firestore/issues/194)) ([c867bd5](https://www.github.com/googleapis/java-firestore/commit/c867bd5772aa4a4710c622546e69fdc0f1ca22b6))
* update jackson dependencies to v2.11.0 ([#195](https://www.github.com/googleapis/java-firestore/issues/195)) ([5066812](https://www.github.com/googleapis/java-firestore/commit/50668126e99422cc9498b317c9c76a80a8bf7b30))
* update protobuf.version to v3.12.0 ([#220](https://www.github.com/googleapis/java-firestore/issues/220)) ([2c0b35d](https://www.github.com/googleapis/java-firestore/commit/2c0b35dfc5786b986b5301a00f06177f527496c3))
* update protobuf.version to v3.12.2 ([#226](https://www.github.com/googleapis/java-firestore/issues/226)) ([2eeea19](https://www.github.com/googleapis/java-firestore/commit/2eeea193d7eb54b1efa92b4d5dd996c170048a73))


### Documentation

* update README to include code formatting ([#209](https://www.github.com/googleapis/java-firestore/issues/209)) ([04f8b3b](https://www.github.com/googleapis/java-firestore/commit/04f8b3b0f873c2f1988c184de1e5268e0de9053f))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants