chore: Extract internal connector registry implementation #1645

hessjcg · 2023-10-25T17:55:03Z

Move the connector registry implementation out of CoreSocketFactory and into InternalConnectorRegistry.

CoreSocketFactory is deprecated and contains only the static public methods used informally by other projects.
It delegates to the InternalConnectorRegistry.

ttosta-google

LGTM

enocom

Let's frame this as "extract internal-only registry" as we're leaving CoreSocketFactory in place.

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

enocom · 2023-10-26T21:48:17Z

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

+ * Factory responsible for obtaining an ephemeral certificate, if necessary, and establishing a
+ * secure connecting to a Cloud SQL instance.
+ *
+ * <p>This class should not be used directly, but only through the JDBC driver specific {@code


Let's make it crystal clear: "Internal class made public for module usage only. This is not part of the official public API and should not be used directly." Or something similar.

Perhaps with a "WARNING" or similar call-out to lead in.

@hessjcg let's add some more warnings to the doc string.

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

core/src/test/java/com/google/cloud/sql/core/CloudSqlConnectorRegistryTest.java

enocom · 2023-10-26T21:55:50Z

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

+  }
+
+  /** Returns the {@link CloudSqlConnectorRegistry} singleton. */
+  public static synchronized CloudSqlConnectorRegistry getInstance() {


Does this need to be public? I know we're going to turn this into an enum as we have in AlloyDB, but shall we keep the API surface as small as possible during that refactor?

This needs to be public so that it can be accessed by com.google.cloud.sql.ConnectorRegistry, which is in a different package. We can look into restricting external access to the core package using Java Modules some day.

OK that's fine. Let's make it clear that this is a "private" method in that case with a similar warning.

enocom · 2023-10-26T21:57:56Z

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

+   *
+   * @throws IllegalStateException if the SQLAdmin client has already been initialized
+   */
+  public static void setApplicationName(String applicationName) {


What shall we do about this? I forget what the plan here was.

We're going to deprecate this method in favor of ConnectorRegistry.addApplicationId(String id).

enocom

LGTM

I'd like to see some serious warnings around our public methods on the InternalConnectorRegistry before we merge this.

hessjcg · 2023-10-30T18:29:20Z

I'd like to see some serious warnings around our public methods on the InternalConnectorRegistry before we merge this.

I added warnings in a few places about internal classes.

enocom · 2023-10-30T20:34:42Z

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

- * SocketFactory} implementations.
- *
- * <p>The API of this class is subject to change without notice.
+ * @deprecated This will soon be replaced.


Replaced with what? Can we be more specific so if someone sees this they'll know what to look for and do instead?

enocom · 2023-10-30T20:35:07Z

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

-    HttpRequestInitializer credential = instanceCredentialFactory.create();
-    DefaultConnectionInfoRepository adminApi =
-        connectionInfoRepositoryFactory.create(credential, config);
+  @Deprecated


Should we mention what people should do instead?

enocom · 2023-10-30T20:36:36Z

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


 /**
- * Factory responsible for obtaining an ephemeral certificate, if necessary, and establishing a
- * secure connecting to a Cloud SQL instance.
+ * WARNING: This class is not a stable, public Java API. The class definition may change without


I think we put this in the wrong place -- this needs to be on the InternalConnectorRegistry.

The bit about compatibility should stay.

enocom · 2023-10-30T20:38:43Z

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

+import jnr.unixsocket.UnixSocketChannel;
+
+/**
+ * Factory responsible for obtaining an ephemeral certificate, if necessary, and establishing a


This isn't a factory. It's a holder of registered connectors. All the connection related code will move into a Connector in future commits.

enocom · 2023-10-30T20:39:37Z

core/src/main/java/com/google/cloud/sql/core/package-info.java

 * change without notice.
+ *
+ * <p>Package com.google.cloud.sql.core holds internal shared packages that implement logic to


Let's add a note about com.google.cloud.sql as holding all the public API.

product-auto-label bot added size: l labels Oct 25, 2023

hessjcg force-pushed the rename-4-CoreSocketFactory branch from 0aebaac to d0a45f8 Compare October 25, 2023 19:32

hessjcg force-pushed the rename-3-ConnectionInfo branch from 3b08337 to 5bad2c6 Compare October 25, 2023 19:32

hessjcg force-pushed the rename-4-CoreSocketFactory branch from d0a45f8 to 3f48b01 Compare October 25, 2023 19:34

hessjcg changed the title ~~refactor: rename CoreSocketFactory to CloudSqlConnectorRegistry.~~ chore: rename CoreSocketFactory to CloudSqlConnectorRegistry. Oct 25, 2023

github-actions bot removed api: cloudsql labels Oct 25, 2023

hessjcg force-pushed the rename-4-CoreSocketFactory branch from 3f48b01 to ffd79e4 Compare October 25, 2023 20:00

hessjcg force-pushed the rename-3-ConnectionInfo branch from 5bad2c6 to dce5da6 Compare October 25, 2023 20:00

product-auto-label bot added the size: l label Oct 25, 2023

hessjcg force-pushed the rename-3-ConnectionInfo branch from dce5da6 to 44fa854 Compare October 25, 2023 20:15

hessjcg force-pushed the rename-4-CoreSocketFactory branch from ffd79e4 to d96b84d Compare October 25, 2023 20:23

Base automatically changed from rename-3-ConnectionInfo to main October 25, 2023 20:56

github-actions bot removed the size: l label Oct 25, 2023

hessjcg force-pushed the rename-4-CoreSocketFactory branch from d96b84d to e1064d0 Compare October 25, 2023 21:05

product-auto-label bot added the size: l label Oct 25, 2023

hessjcg marked this pull request as ready for review October 25, 2023 21:05

hessjcg requested a review from a team as a code owner October 25, 2023 21:05

blunderbuss-gcf bot assigned ttosta-google Oct 25, 2023

product-auto-label bot added the api: cloudsql label Oct 26, 2023

ttosta-google approved these changes Oct 26, 2023

View reviewed changes

enocom self-requested a review October 26, 2023 21:41

enocom requested changes Oct 26, 2023

View reviewed changes

enocom reviewed Oct 26, 2023

View reviewed changes

core/src/main/java/com/google/cloud/sql/core/CloudSqlConnectorRegistry.java Outdated Show resolved Hide resolved

enocom reviewed Oct 26, 2023

View reviewed changes

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java Outdated Show resolved Hide resolved

enocom reviewed Oct 26, 2023

View reviewed changes

core/src/test/java/com/google/cloud/sql/core/CloudSqlConnectorRegistryTest.java Outdated Show resolved Hide resolved

enocom reviewed Oct 26, 2023

View reviewed changes

hessjcg force-pushed the rename-4-CoreSocketFactory branch from e1064d0 to 37b59c7 Compare October 27, 2023 20:35

hessjcg changed the title ~~chore: rename CoreSocketFactory to CloudSqlConnectorRegistry.~~ chore: rename CoreSocketFactory to InternalConnectorRegistry. Oct 27, 2023

hessjcg changed the title ~~chore: rename CoreSocketFactory to InternalConnectorRegistry.~~ chore: Extract internal connector registry implementation from CoreSocketFactory. Oct 27, 2023

hessjcg force-pushed the rename-4-CoreSocketFactory branch from 37b59c7 to b31f67d Compare October 27, 2023 21:09

hessjcg requested a review from enocom October 27, 2023 21:19

github-actions bot removed size: l labels Oct 30, 2023

enocom reviewed Oct 30, 2023

View reviewed changes

product-auto-label bot added the size: l label Oct 30, 2023

hessjcg requested a review from enocom October 30, 2023 18:28

hessjcg added 2 commits October 30, 2023 13:27

refactor: rename CoreSocketConnector to CloudSqlConnectorRegistry.

5fa87ff

docs: Add warning about internal classes.

a5d2892

hessjcg force-pushed the rename-4-CoreSocketFactory branch from 8e155fc to a5d2892 Compare October 30, 2023 19:27

enocom changed the title ~~chore: Extract internal connector registry implementation from CoreSocketFactory.~~ chore: Extract internal connector registry implementation Oct 30, 2023

enocom approved these changes Oct 30, 2023

View reviewed changes

hessjcg enabled auto-merge (squash) October 30, 2023 20:47

docs: better warning messages.

24bde1a

hessjcg force-pushed the rename-4-CoreSocketFactory branch from ff6cf84 to 24bde1a Compare October 30, 2023 20:47

hessjcg merged commit af37fb3 into main Oct 30, 2023

hessjcg deleted the rename-4-CoreSocketFactory branch October 30, 2023 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Extract internal connector registry implementation #1645

chore: Extract internal connector registry implementation #1645

hessjcg commented Oct 25, 2023 •

edited

Loading

ttosta-google left a comment

enocom left a comment •

edited

Loading

enocom Oct 26, 2023

enocom Oct 26, 2023

enocom Oct 30, 2023

enocom Oct 26, 2023

hessjcg Oct 27, 2023

enocom Oct 30, 2023

enocom Oct 26, 2023

hessjcg Oct 27, 2023

enocom left a comment

hessjcg commented Oct 30, 2023

enocom Oct 30, 2023

enocom Oct 30, 2023

enocom Oct 30, 2023

enocom Oct 30, 2023

enocom Oct 30, 2023

chore: Extract internal connector registry implementation #1645

chore: Extract internal connector registry implementation #1645

Conversation

hessjcg commented Oct 25, 2023 • edited Loading

ttosta-google left a comment

Choose a reason for hiding this comment

enocom left a comment • edited Loading

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

hessjcg commented Oct 30, 2023

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 Oct 25, 2023 •

edited

Loading

enocom left a comment •

edited

Loading