-
Notifications
You must be signed in to change notification settings - Fork 119
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: Use guava rate limiter instead of dev.failsafe #1393
Conversation
Can we make the base branch main? |
@@ -71,7 +70,7 @@ class CloudSqlInstance { | |||
private boolean forceRefreshRunning; | |||
|
|||
static final RateLimiter defaultRateLimiter() { | |||
return RateLimiter.burstyBuilder(2, Duration.ofSeconds(30)).build(); | |||
return RateLimiter.create(2.0 / 60.0); // 2 requests/minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want 1 request every 30 seconds to mostly match the other connectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 request every 30 seconds is exactly what this means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not write 30.0 then? Also the comment is misleading.
Looking closer -- why not 1.0 / 30.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original PR from long ago had 2.0/60.0. I have updated this PR for clarity to be 1.0/30.0.
8a19d1b
to
18e4324
Compare
Looks like the base branch did change, but now we're stuck with a bunch of conflicts. In the future, we can always make main the base branch, I can approve work ahead of time and merge conflicts can be handled async. |
c1fbbfd
to
bdcab1d
Compare
@@ -359,6 +358,6 @@ CloudSqlInstance getCloudSqlInstance(String instanceName, AuthType authType) { | |||
credentialFactory, | |||
executor, | |||
localKeyPair, | |||
RateLimiter.burstyBuilder(2, Duration.ofSeconds(30)).build())); | |||
RateLimiter.create(1.0 / 30.0))); // 1 refresh attempt every 30 seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize now why the original code had 2.0 / 60.0
-- the idea was to allow an immediate second request if the first failed for some reason (hence the bursty rate limiter in the other connectors). We can approximate that here with 2.0 / 60.0
. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this makes sense. It makes no difference whether we write RateLimiter.create(1.0/30.0)
or RateLimiter.create(2.0/60.0)
in the code. Both statements will be compiled to RateLimiter.create(0.03333333333)
because Java computes constant expressions at compile time. All these statements create a rate limiter that allows 0.03333333333 requests per second. There is no way to configure a burst in the Guava rate limiter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 yes of course
Then I have no idea why it was 2.0 / 60.0
.
private RateLimiter<Object> newRateLimiter() { | ||
return RateLimiter.burstyBuilder(2, Duration.ofMillis(50)).build(); | ||
private RateLimiter newRateLimiter() { | ||
return RateLimiter.create(20.0); // 20/sec = every 50 ms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this match our production code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The concurrency tests reduce length of both modeled response times and timeouts from real behavior so that the tests run faster and we get more "shots on goal" to produce a deadlock or other threading problem.
bdcab1d
to
608cae4
Compare
For simplicity, we are removing the library dev.failsafe and using a rate limiter already included in
the Guava library instead.