refactor: Refactor RefreshAheadConnectionInfoCache. Part of #992. #1997

hessjcg · 2024-05-23T20:27:37Z

This makes a number of refactoring changes to align the Java connector with other implementations.

Introduce a ConnectionInfoCache interface
Rename DefaultConnectionInfoCache to RefreshAheadConnectionInfoCache
Update and simplify instantiation logic of RefreshAheadConnectionInfoCache

hessjcg · 2024-05-23T20:32:11Z

core/src/main/java/com/google/cloud/sql/core/RefreshAheadConnectionInfoCache.java

+   * @param executor executor used to schedule asynchronous tasks
+   * @param keyPair public/private key pair used to authenticate connections
+   */
+  public static RefreshAheadConnectionInfoCache newInstance(


I have to use a static factory method instead of a constructor because it has to instantiate an AccessTokenSupplier and RefreshAheadStrategy before it calls the constructor.,

enocom

What if the ConnectinInfoCache just returned the connection info instead of creating SSL sockets? Recently, in Go and Python we've been pulling apart the thing that caches connection info (e.g., the "cache") from the thing that creates SSL sockets (e.g., the "connector"). That's what we're doing in AlloyDB and it's working well: https://github.com/GoogleCloudPlatform/alloydb-java-connector/blob/main/alloydb-jdbc-connector/src/main/java/com/google/cloud/alloydb/ConnectionInfoCache.java

hessjcg · 2024-05-24T20:46:16Z

I'll remove the createSslSocket() method from the ConnectionInfoCache. It really is only a convenience method anyway.

enocom · 2024-05-24T21:47:31Z

core/src/main/java/com/google/cloud/sql/core/BaseConnectionInfoCache.java

-    this.instanceName = new CloudSqlInstanceName(config.getCloudSqlInstance());
-    this.config = config;
-
+abstract class BaseConnectionInfoCache implements ConnectionInfoCache {


The base class is delegating all calls to the refresh strategy. What if we avoid introducing more indirection and just had the implementations of ConnectionInfoCache call through to their refresher?

I removed the base class.

enocom · 2024-05-24T21:48:21Z

core/src/main/java/com/google/cloud/sql/core/ConnectionInfoCache.java

+
+  RefreshStrategy getRefresher();
+
+  CloudSqlInstanceName getInstanceName();


This is used only in test. Let's remove the method here and either drop the usages in test or find a more direct way at getting at the information the test wants.

I removed this method too.

enocom · 2024-05-24T21:48:47Z

core/src/main/java/com/google/cloud/sql/core/ConnectionInfoCache.java

+
+  void refreshIfExpired();
+
+  RefreshStrategy getRefresher();


This is a test-only method. How can we avoid adding it here?

I've removed it from the interface.

enocom · 2024-05-24T21:50:21Z

core/src/main/java/com/google/cloud/sql/core/RefreshAheadConnectionInfoCache.java

+      long minRefreshDelayMs) {
+
+    AccessTokenSupplier accessTokenSupplier =
+        BaseConnectionInfoCache.newAccessTokenSupplier(config, tokenSourceFactory);


This static method is making our lives harder. Why don't we pass in the access token supplier from where we instantiate the cache?

Refactored, removing the base class, so that this can be a normal constructor.

hessjcg · 2024-05-28T17:29:13Z

core/src/main/java/com/google/cloud/sql/core/ConnectionInfo.java

@@ -51,4 +52,29 @@ Map<IpType, String> getIpAddrs() {
  SslData getSslData() {
    return sslData;
  }
+
+  ConnectionMetadata toConnectionMetadata(


Moved here because BaseConnectionInfoCache was deleted.

(Additional context: ConnectionMetadata is part of the legacy public Java API contract.)

This seems like the perfect place for such concerns -- basically the connection info data class.

hessjcg · 2024-05-28T17:39:27Z

core/src/main/java/com/google/cloud/sql/core/DefaultAccessTokenSupplier.java

@@ -43,6 +44,14 @@ class DefaultAccessTokenSupplier implements AccessTokenSupplier {
  private final int retryCount;
  private final Duration retryDuration;

+  static AccessTokenSupplier newInstance(AuthType authType, CredentialFactory tokenSourceFactory) {


Moved here because BaseConnectionInfoCache was deleted.

Perfect spot.

enocom

There's one use of public to probably remove. Otherwise LGTM.

enocom · 2024-05-28T17:50:26Z

core/src/main/java/com/google/cloud/sql/core/ConnectionInfo.java

@@ -51,4 +52,29 @@ Map<IpType, String> getIpAddrs() {
  SslData getSslData() {
    return sslData;
  }
+
+  ConnectionMetadata toConnectionMetadata(


This seems like the perfect place for such concerns -- basically the connection info data class.

enocom · 2024-05-28T17:50:43Z

core/src/main/java/com/google/cloud/sql/core/ConnectionInfoCache.java

+package com.google.cloud.sql.core;
+
+/** ConnectionInfoCache is the contract for a caching strategy for ConnectionInfo. */
+public interface ConnectionInfoCache {


Do we want this to be public?

I removed public.

enocom · 2024-05-28T17:51:34Z

core/src/main/java/com/google/cloud/sql/core/DefaultAccessTokenSupplier.java

@@ -43,6 +44,14 @@ class DefaultAccessTokenSupplier implements AccessTokenSupplier {
  private final int retryCount;
  private final Duration retryDuration;

+  static AccessTokenSupplier newInstance(AuthType authType, CredentialFactory tokenSourceFactory) {


Perfect spot.

hessjcg requested a review from a team as a code owner May 23, 2024 20:27

blunderbuss-gcf bot assigned jackwotherspoon May 23, 2024

hessjcg requested review from enocom and jackwotherspoon May 23, 2024 20:28

hessjcg mentioned this pull request May 23, 2024

feat: Add lazy refresh strategy to the connector. Fixes #992. #1990

Merged

hessjcg commented May 23, 2024

View reviewed changes

hessjcg force-pushed the gh-992-refactor-refresh-ahead-cache branch from e0fa4bb to e9708f6 Compare May 23, 2024 20:34

enocom reviewed May 23, 2024

View reviewed changes

jackwotherspoon changed the title ~~chore: Refactor RefreshAheadConnectionInfoCache. Part of #992.~~ refactor: Refactor RefreshAheadConnectionInfoCache. Part of #992. May 23, 2024

hessjcg force-pushed the gh-992-refactor-refresh-ahead-cache branch from e9708f6 to 3716afd Compare May 24, 2024 20:52

hessjcg requested a review from enocom May 24, 2024 20:52

enocom requested changes May 24, 2024

View reviewed changes

hessjcg force-pushed the gh-992-refactor-refresh-ahead-cache branch 3 times, most recently from 6143994 to 84ef897 Compare May 28, 2024 17:44

hessjcg commented May 28, 2024

View reviewed changes

hessjcg requested a review from enocom May 28, 2024 17:44

enocom approved these changes May 28, 2024

View reviewed changes

hessjcg force-pushed the gh-992-refactor-refresh-ahead-cache branch from 84ef897 to 86a443e Compare May 28, 2024 19:47

hessjcg enabled auto-merge (squash) May 28, 2024 19:48

hessjcg disabled auto-merge May 28, 2024 19:48

hessjcg added 3 commits May 29, 2024 09:02

chore: Rename DefaultConnectionInfoCache to BaseConnectionInfoCache.

bfee6e2

chore: Introduce ConnectionInfoCache interface (part of #992).

11b25eb

ALT-1 chore: Refactor RefreshAheadConnectionInfoCache. Part of #992.

f7de7c0

hessjcg force-pushed the gh-992-refactor-refresh-ahead-cache branch from 86a443e to f7de7c0 Compare May 29, 2024 15:03

hessjcg enabled auto-merge (squash) May 29, 2024 15:04

hessjcg merged commit d97a93b into main May 29, 2024
17 checks passed

hessjcg deleted the gh-992-refactor-refresh-ahead-cache branch May 29, 2024 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Refactor RefreshAheadConnectionInfoCache. Part of #992. #1997

refactor: Refactor RefreshAheadConnectionInfoCache. Part of #992. #1997

hessjcg commented May 23, 2024 •

edited

Loading

hessjcg May 23, 2024

enocom left a comment

hessjcg commented May 24, 2024

enocom May 24, 2024

hessjcg May 28, 2024

enocom May 24, 2024

hessjcg May 28, 2024

enocom May 24, 2024

hessjcg May 28, 2024

enocom May 24, 2024

hessjcg May 28, 2024

hessjcg May 28, 2024

enocom May 28, 2024

hessjcg May 28, 2024

enocom May 28, 2024

enocom left a comment

enocom May 28, 2024

enocom May 28, 2024

hessjcg May 28, 2024

enocom May 28, 2024


		RefreshStrategy getRefresher();

		CloudSqlInstanceName getInstanceName();

refactor: Refactor RefreshAheadConnectionInfoCache. Part of #992. #1997

refactor: Refactor RefreshAheadConnectionInfoCache. Part of #992. #1997

Conversation

hessjcg commented May 23, 2024 • edited Loading

Choose a reason for hiding this comment

enocom left a comment

Choose a reason for hiding this comment

hessjcg commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enocom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hessjcg commented May 23, 2024 •

edited

Loading