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.

Sign up for GitHub

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

Jump to bottom

fix: Use guava rate limiter instead of dev.failsafe #1393

Merged
merged 1 commit into from
Jul 25, 2023
Merged

fix: Use guava rate limiter instead of dev.failsafe #1393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,6 @@
</dependencyManagement>

<dependencies>
<dependency>
<groupId>dev.failsafe</groupId>
<artifactId>failsafe</artifactId>
<version>3.3.2</version>
</dependency>

<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
Expand Down
8 changes: 4 additions & 4 deletions core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningScheduledExecutorService;
import com.google.common.util.concurrent.RateLimiter;
import com.google.common.util.concurrent.Uninterruptibles;
import com.google.errorprone.annotations.concurrent.GuardedBy;
import dev.failsafe.RateLimiter;
import java.io.IOException;
import java.security.KeyPair;
import java.time.Instant;
Expand Down Expand Up @@ -56,7 +56,7 @@ class CloudSqlInstance {
private final ListenableFuture<KeyPair> keyPair;
private final Object instanceDataGuard = new Object();
// Limit forced refreshes to 1 every minute.
private final RateLimiter<Object> forcedRenewRateLimiter;
private final RateLimiter forcedRenewRateLimiter;

private final RefreshCalculator refreshCalculator = new RefreshCalculator();

Expand Down Expand Up @@ -84,7 +84,7 @@ class CloudSqlInstance {
CredentialFactory tokenSourceFactory,
ListeningScheduledExecutorService executor,
ListenableFuture<KeyPair> keyPair,
RateLimiter<Object> forcedRenewRateLimiter) {
RateLimiter forcedRenewRateLimiter) {
this.instanceName = new CloudSqlInstanceName(connectionName);
this.instanceDataSupplier = instanceDataSupplier;
this.authType = authType;
Expand Down Expand Up @@ -189,7 +189,7 @@ private InstanceData performRefresh() throws InterruptedException, ExecutionExce
logger.fine(
String.format("[%s] Refresh Operation: Acquiring rate limiter permit.", instanceName));
// To avoid unreasonable SQL Admin API usage, use a rate limit to throttle our usage.
forcedRenewRateLimiter.acquirePermit();
forcedRenewRateLimiter.acquire();
logger.fine(
String.format(
"[%s] Refresh Operation: Acquired rate limiter permit. Starting refresh...",
Expand Down
5 changes: 2 additions & 3 deletions core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,14 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningScheduledExecutorService;
import com.google.common.util.concurrent.MoreExecutors;
import dev.failsafe.RateLimiter;
import com.google.common.util.concurrent.RateLimiter;
import java.io.File;
import java.io.IOException;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.security.KeyPair;
import java.security.KeyPairGenerator;
import java.security.NoSuchAlgorithmException;
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
import java.util.Properties;
Expand Down Expand Up @@ -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
Copy link
Member

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?

Copy link
Collaborator Author

@hessjcg hessjcg Jul 21, 2023 •
edited
Loading

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.

Copy link
Member

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.

}
}
7 changes: 3 additions & 4 deletions core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Copy link
Member

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?

Copy link
Collaborator Author

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.

}
}
13 changes: 6 additions & 7 deletions core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,9 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningScheduledExecutorService;
import com.google.common.util.concurrent.MoreExecutors;
import dev.failsafe.RateLimiter;
import com.google.common.util.concurrent.RateLimiter;
import java.security.KeyPair;
import java.sql.Date;
import java.time.Duration;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.Arrays;
Expand Down Expand Up @@ -77,7 +76,7 @@ public void testCloudSqlInstanceDataRetrievedSuccessfully() throws Exception {
stubCredentialFactory,
executorService,
keyPairFuture,
RateLimiter.burstyBuilder(2, Duration.ofSeconds(30)).build());
RateLimiter.create(1.0 / 30.0));

SslData gotSslData = instance.getSslData();
assertThat(gotSslData).isSameInstanceAs(instanceDataSupplier.response.getSslData());
Expand Down Expand Up @@ -111,7 +110,7 @@ public void testInstanceFailsOnConnectionError() throws Exception {
stubCredentialFactory,
executorService,
keyPairFuture,
RateLimiter.burstyBuilder(2, Duration.ofSeconds(30)).build());
RateLimiter.create(1.0 / 30.0));

RuntimeException ex = Assert.assertThrows(RuntimeException.class, instance::getSslData);
assertThat(ex).hasMessageThat().contains("always fails");
Expand Down Expand Up @@ -139,7 +138,7 @@ public void testCloudSqlInstanceForcesRefresh() throws Exception {
stubCredentialFactory,
executorService,
keyPairFuture,
RateLimiter.burstyBuilder(2, Duration.ofSeconds(30)).build());
RateLimiter.create(1.0 / 30.0));

SslData gotSslData = instance.getSslData();
assertThat(gotSslData).isSameInstanceAs(sslData);
Expand Down Expand Up @@ -179,7 +178,7 @@ public void testGetPreferredIpTypes() throws Exception {
stubCredentialFactory,
executorService,
keyPairFuture,
RateLimiter.burstyBuilder(2, Duration.ofSeconds(30)).build());
RateLimiter.create(1.0 / 30.0));

assertThat(instance.getPreferredIp(Arrays.asList("PUBLIC", "PRIVATE"))).isEqualTo("10.1.2.3");
assertThat(instance.getPreferredIp(Arrays.asList("PUBLIC"))).isEqualTo("10.1.2.3");
Expand Down Expand Up @@ -216,7 +215,7 @@ public void testGetPreferredIpTypesThrowsException() throws Exception {
stubCredentialFactory,
executorService,
keyPairFuture,
RateLimiter.burstyBuilder(2, Duration.ofSeconds(30)).build());
RateLimiter.create(1.0 / 30.0));
Assert.assertThrows(
IllegalArgumentException.class, () -> instance.getPreferredIp(Arrays.asList("PRIVATE")));
}
Expand Down