feat(spanner): add support for external hosts #3474

sagnghos · 2024-11-14T15:50:46Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

conventional-commit-lint-gcf · 2024-11-14T15:50:52Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

krishnamoorthy-r · 2024-11-14T16:22:59Z

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

@@ -49,10 +51,16 @@ private SessionId(DatabaseId db, String name) {
    static SessionId of(String name) {
      Preconditions.checkNotNull(name);
      Map<String, String> parts = NAME_TEMPLATE.match(name);
+      if (parts == null) {
+        parts = EXTERNAL_HOST_NAME_TEMPLATE.match(name);


Can you add unit tests to cover this pattern and ensure the functionality works as intended?

Added unit test to cover existing pattern (for regression) as well as new pattern

krishnamoorthy-r · 2024-11-14T16:24:22Z

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

@@ -94,6 +94,11 @@ public static DatabaseId of(String project, String instance, String database) {
    return new DatabaseId(new InstanceId(project, instance), database);
  }

+  /** Creates a {@code DatabaseId} given instance and database IDs. */
+  public static DatabaseId of(String instance, String database) {
+    return new DatabaseId(new InstanceId("default", instance), database);


I think we should document to users that this method assumes instance Id to be "default".

maybe not instance Id, but rather project Id?

krishnamoorthy-r · 2024-11-15T08:19:02Z

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

@@ -94,6 +94,11 @@ public static DatabaseId of(String project, String instance, String database) {
    return new DatabaseId(new InstanceId(project, instance), database);
  }

+  /** Creates a {@code DatabaseId} with "default" project, given instance and database IDs. */


nit nit pick: "default" as project id.

krishnamoorthy-r · 2024-11-15T08:20:47Z

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

+    try {
+      SessionId.of(host);
+      SessionId.of(externalHost);
+      // If no exceptions are thrown, the test will pass


Is it possible to add assertEquas here to validate all the attributes instanceid, projectid and databaseid as set in both the scenarios?

added assert for session name.

…default

olavloite

LGTM, with a couple of tiny nits.

olavloite · 2024-11-15T16:14:10Z

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

@@ -94,6 +94,15 @@ public static DatabaseId of(String project, String instance, String database) {
    return new DatabaseId(new InstanceId(project, instance), database);
  }

+  /** Creates a {@code DatabaseId} with "default" as project id, given instance and database IDs. */


Suggested change

/** Creates a {@code DatabaseId} with "default" as project id, given instance and database IDs. */

/** Creates a {@code DatabaseId} with the default project ID of this environment and the given instance and database IDs. The project ID is set to "default" if no project ID could be found for this environment. */

olavloite · 2024-11-15T16:16:46Z

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

+  /** Creates a {@code DatabaseId} with "default" as project id, given instance and database IDs. */
+  public static DatabaseId of(String instance, String database) {
+    String projectId = SpannerOptions.getDefaultProjectId();
+    if (projectId == null) {


nit: maybe use Strings.isNullOrEmpty(String) instead. That prevents an empty string from being used as the project ID.

olavloite · 2024-11-19T18:55:06Z

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

+      SessionId externalHostSession = SessionId.of(externalHost);
+      assertEquals("abcd1234", externalHostSession.getName());
+      // If no exceptions are thrown, the test will pass
+    } catch (IllegalArgumentException e) {


nit: you can remove the entire try-catch block. If a (runtime) exception occurs during a test, then this test will be marked as failed with the error message. I.e. you will get the exact same behavior as you have now with the try-catch block, but with a bit less code and indentation.

sagnghos requested a review from a team as a code owner November 14, 2024 15:50

product-auto-label bot added size: s Pull request size is small. api: spanner Issues related to the googleapis/java-spanner API. labels Nov 14, 2024

feat(spanner): add support for external hosts

f1b5719

sagnghos force-pushed the sagnikghosh-spanner branch from a09b78b to f1b5719 Compare November 14, 2024 15:57

krishnamoorthy-r reviewed Nov 14, 2024

View reviewed changes

feat(spanner): added unit test

0a74b9c

krishnamoorthy-r suggested changes Nov 15, 2024

View reviewed changes

feat(spanner): added asserts to test

786e764

rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 15, 2024

krishnamoorthy-r approved these changes Nov 15, 2024

View reviewed changes

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 15, 2024

rahul2393 approved these changes Nov 15, 2024

View reviewed changes

feat(spanner): added code to fetch default project id before setting …

7fc9367

…default

olavloite approved these changes Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(spanner): add support for external hosts #3474

feat(spanner): add support for external hosts #3474

sagnghos commented Nov 14, 2024 •

edited

Loading

conventional-commit-lint-gcf bot commented Nov 14, 2024 •

edited

Loading

krishnamoorthy-r Nov 14, 2024

sagnghos Nov 15, 2024

krishnamoorthy-r Nov 14, 2024

ko3a4ok Nov 15, 2024

krishnamoorthy-r Nov 15, 2024

krishnamoorthy-r Nov 15, 2024

sagnghos Nov 15, 2024

olavloite left a comment

olavloite Nov 15, 2024

olavloite Nov 15, 2024

olavloite Nov 19, 2024

	/** Creates a {@code DatabaseId} with "default" as project id, given instance and database IDs. */
	/** Creates a {@code DatabaseId} with the default project ID of this environment and the given instance and database IDs. The project ID is set to "default" if no project ID could be found for this environment. */

feat(spanner): add support for external hosts #3474

Are you sure you want to change the base?

feat(spanner): add support for external hosts #3474

Conversation

sagnghos commented Nov 14, 2024 • edited Loading

conventional-commit-lint-gcf bot commented Nov 14, 2024 • 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

olavloite 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

sagnghos commented Nov 14, 2024 •

edited

Loading

conventional-commit-lint-gcf bot commented Nov 14, 2024 •

edited

Loading