feat: support option for setting client_id while creating spanner database client by aakashanandg · Pull Request #3832 · googleapis/java-spanner

aakashanandg · 2025-04-23T12:50:32Z

Fixes googleapis/java-spanner-jdbc#1856.

Summary:

This PR adds support for specifying an optional client_Id when creating a Spanner DatabaseClient. This allows client applications (including JDBC connections using the client_id property) to identify themselves to the Spanner service, primarily for enhanced monitoring and logging capabilities.

Implementation:

Added the getDatabaseClient(DatabaseId db, String clientId) overload to the Spanner interface.
Modified connection setup logic to parse and utilize the clientId when obtaining the DatabaseClient.

Testing:

All unit tests pass (mvn test).

olavloite · 2025-04-25T07:27:57Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java

-    this.dbClient = spanner.getDatabaseClient(options.getDatabaseId());
+    DatabaseClient tempDbClient = null;
+    final DatabaseId databaseId = options.getDatabaseId();
+    try {
+      Optional<String> clientIdOpt = extractClientIdOptional(options);
+      if (clientIdOpt.isPresent() && !clientIdOpt.get().isEmpty()) {
+        if (this.spanner instanceof ExtendedSpanner) {
+          ExtendedSpanner extendedSpanner = (ExtendedSpanner) this.spanner;
+          tempDbClient = extendedSpanner.getDatabaseClient(databaseId, clientIdOpt.get());
+        }
+      }
+    } catch (Exception e) {
+      System.err.println(
+          "WARNING: Failed during DatabaseClient initialization (possibly getting specific ID), falling back to default. Error: "
+              + e.getMessage());
+    }
+    if (tempDbClient == null) {
+      tempDbClient = spanner.getDatabaseClient(databaseId);
+    }
+    this.dbClient = tempDbClient;


You can simplify this entire block into one line like this:

this.dbClient = spanner.getDatabaseClient( databaseId, options.getInitialConnectionPropertyValue(ConnectionProperties.CLIENT_ID));

But to do this, you need to make sure that:

getDatabaseClient(DatabaseId, String) accepts a null value for the second argument (which it should, as that is the default when calling the overload without a client id)

Change the return type of SpannerPool#getSpanner(..) to an interface or class that supports the additional client-id argument. That means that you get an instance that you can use directly on line 315.

olavloite · 2025-04-25T07:29:42Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java

+  private Optional<String> extractClientIdOptional(ConnectionOptions options) {
+    return Optional.ofNullable(options.getInitialConnectionPropertyValues())
+        .map(props -> props.get(CLIENT_ID))
+        .map(ConnectionPropertyValue::getValue)
+        .map(Object::toString)
+        .filter(id -> !id.isEmpty());
+  }
+


Remove, see above

olavloite · 2025-04-25T07:29:55Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java


  @VisibleForTesting
-  ConnectionState.Type getConnectionStateType() {
+  Type getConnectionStateType() {


Remove the unrelated changes in this PR

olavloite · 2025-04-25T07:30:28Z

...le-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionProperties.java

+          null,
+          StringValueConverter.INSTANCE,
+          Context.STARTUP);
+


nit: remove empty line

olavloite · 2025-04-25T07:31:32Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java

+
+    DatabaseClientImpl databaseClient =
+        (DatabaseClientImpl) impl.getDatabaseClient(db, "clientId-1");
+    assertThat(databaseClient.clientId).isEqualTo("clientId-1");


nit: prefer the use of junit assertions, so in this case assertEquals(expected, actual). This should also be the case if the test class that you are editing contains assertThat(..) style assertions.

olavloite · 2025-04-25T07:34:24Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java

  private static final ParsedStatement RELEASE_STATEMENT =
      AbstractStatementParser.getInstance(Dialect.GOOGLE_STANDARD_SQL)
          .parse(Statement.of("RELEASE s1"));
+  private static final String CLIENT_ID = "client_id";


nit: remove (see below)

olavloite · 2025-04-25T07:42:25Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java

+  }
+
+  @Override
+  public DatabaseClient getDatabaseClient(DatabaseId db, String clientId) {


What happens if you call this method twice with the same db and clientId? Does it return the same instance, both with the correct clientId? Add a test for that.

What happens if you call this method twice with the same db, but different clientIds? Does it return the same instance? Or two different instances with the correct clientId? Add a test for that.

What happens if you call this method twice with different dbs, but the same clientIds? Add a test.

olavloite · 2025-04-25T07:43:42Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java

+    // Getting a new database client for an invalidated database should use the same client id.
+    databaseClient.pool.setResourceNotFoundException(
+        new DatabaseNotFoundException(DoNotConstructDirectly.ALLOWED, "not found", null, null));
+    DatabaseClientImpl revalidated = (DatabaseClientImpl) impl.getDatabaseClient(db, "clientId-1");
+    assertThat(revalidated).isNotSameInstanceAs(databaseClient);
+    assertThat(revalidated.clientId).isEqualTo(databaseClient.clientId);


This seems like a copy-pasted test from something else that is not really testing the new feature that is being added here.

olavloite · 2025-04-25T08:08:28Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/ExtendedSpanner.java

+
+package com.google.cloud.spanner;
+
+public interface ExtendedSpanner extends Spanner {


I think that we should:

Move this interface into the package com.google.cloud.spanner.connection

Rename it to InternalSpanner

Add the annotation @InternalApi

Add a javadoc saying '/** This interface is used internally to create (JDBC) connections for Spanner and is only intended for internal usage. */`

olavloite · 2025-04-25T08:09:02Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/ExtendedSpanner.java

+  /**
+   * Returns a {@code DatabaseClient} for the given database and given client id. It uses a pool of
+   * sessions to talk to the database.
+   * <!--SNIPPET get_db_client-->
+   *
+   * <pre>{@code
+   * SpannerOptions options = SpannerOptions.newBuilder().build();
+   * Spanner spanner = options.getService();
+   * final String project = "test-project";
+   * final String instance = "test-instance";
+   * final String database = "example-db";
+   * final String client_id = "client_id"
+   * DatabaseId db =
+   *     DatabaseId.of(project, instance, database);
+   *
+   * DatabaseClient dbClient = spanner.getDatabaseClient(db, client_id);
+   * }</pre>
+   *
+   * <!--SNIPPET get_db_client-->
+   */


Remove this documentation, as this is intended for internal usage. Also add an @InternalApi annotation to the method.

aakashanandg added 4 commits April 23, 2025 16:58

feat: support option for setting client-id

556a2c5

feat: support option for setting client-id

d814755

feat: support option for setting client-id

3c587df

feat: support option for setting client-id

27c1d95

aakashanandg requested review from a team April 23, 2025 12:50

product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/java-spanner API. labels Apr 23, 2025

aakashanandg requested a review from olavloite April 23, 2025 12:50

aakashanandg added 5 commits April 23, 2025 18:56

feat: support option for setting client-id

c770b77

Merge branch 'main' into client-id-branch

13b7d5c

feat: support option for setting client-id

b2079f4

feat: support option for setting client-id

a2f790a

feat: support option for setting client-id

c78ef3d

aakashanandg changed the title ~~feat: support option for setting client_id while creating Spanner DatabaseClient~~ feat: support option for setting client_id while creating spanner database client Apr 24, 2025

olavloite reviewed Apr 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support option for setting client_id while creating spanner database client#3832

feat: support option for setting client_id while creating spanner database client#3832
aakashanandg wants to merge 9 commits intogoogleapis:mainfrom
aakashanandg:client-id-branch

aakashanandg commented Apr 23, 2025 •

edited

Loading

Uh oh!

olavloite Apr 25, 2025

Uh oh!

olavloite Apr 25, 2025

Uh oh!

olavloite Apr 25, 2025

Uh oh!

olavloite Apr 25, 2025

Uh oh!

olavloite Apr 25, 2025

Uh oh!

olavloite Apr 25, 2025

Uh oh!

olavloite Apr 25, 2025

Uh oh!

olavloite Apr 25, 2025

Uh oh!

olavloite Apr 25, 2025

Uh oh!

olavloite Apr 25, 2025

Uh oh!

olavloite Apr 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		package com.google.cloud.spanner;

		public interface ExtendedSpanner extends Spanner {

Conversation

aakashanandg commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aakashanandg commented Apr 23, 2025 •

edited

Loading