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.
We'll want 1 request every 30 seconds to mostly match the other connectors.
There was a problem hiding this comment.
1 request every 30 seconds is exactly what this means.
There was a problem hiding this comment.
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.
The original PR from long ago had 2.0/60.0. I have updated this PR for clarity to be 1.0/30.0.
hessjcg
force-pushed
the
gh-1314-guava-rate-limiter
branch
from
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. |
hessjcg
force-pushed
the
gh-1314-guava-rate-limiter
branch
from
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.
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.
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.
🤦 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.
Shouldn't this match our production code?
There was a problem hiding this comment.
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.
hessjcg
force-pushed
the
gh-1314-guava-rate-limiter
branch
from
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.