-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,10 +25,9 @@ | |
import com.google.common.util.concurrent.Futures; | ||
import com.google.common.util.concurrent.ListenableFuture; | ||
import com.google.common.util.concurrent.ListeningScheduledExecutorService; | ||
import dev.failsafe.RateLimiter; | ||
import com.google.common.util.concurrent.RateLimiter; | ||
import java.io.IOException; | ||
import java.security.KeyPair; | ||
import java.time.Duration; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.concurrent.ExecutionException; | ||
|
@@ -214,7 +213,7 @@ private Thread startForceRefreshThread(CloudSqlInstance inst) { | |
return t; | ||
} | ||
|
||
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 commentThe 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 commentThe 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. |
||
} | ||
} |
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 with2.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)
orRateLimiter.create(2.0/60.0)
in the code. Both statements will be compiled toRateLimiter.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
.