Skip to content

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

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1b5719
feat(spanner): add support for external hosts
sagnghos Nov 14, 2024
0a74b9c
feat(spanner): added unit test
sagnghos Nov 15, 2024
786e764
feat(spanner): added asserts to test
sagnghos Nov 15, 2024
7fc9367
feat(spanner): added code to fetch default project id before setting …
sagnghos Nov 15, 2024
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
5 changes: 5 additions & 0 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseId.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" as project id, given instance and database IDs. */
Copy link
Collaborator

@olavloite olavloite Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. */

public static DatabaseId of(String instance, String database) {
return new DatabaseId(new InstanceId("default", instance), database);
Copy link

@krishnamoorthy-r krishnamoorthy-r Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ko3a4ok ko3a4ok Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe not instance Id, but rather project Id?

}

/** Creates a {@code DatabaseId} given the instance identity and database id. */
public static DatabaseId of(InstanceId instanceId, String database) {
return new DatabaseId(instanceId, database);
Expand Down
10 changes: 9 additions & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ static class SessionId {
private static final PathTemplate NAME_TEMPLATE =
PathTemplate.create(
"projects/{project}/instances/{instance}/databases/{database}/sessions/{session}");
private static final PathTemplate EXTERNAL_HOST_NAME_TEMPLATE =
PathTemplate.create("instances/{instance}/databases/{database}/sessions/{session}");
private final DatabaseId db;
private final String name;

Expand All @@ -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);
Copy link

@krishnamoorthy-r krishnamoorthy-r Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sagnghos sagnghos Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
Preconditions.checkArgument(
parts != null, "Name should conform to pattern %s: %s", NAME_TEMPLATE, name);
return of(
parts.get("project"), parts.get("instance"), parts.get("database"), parts.get("session"));
parts.containsKey("project") ? parts.get("project") : "default",
parts.get("instance"),
parts.get("database"),
parts.get("session"));
}

/** Creates a {@code SessionId} given project, instance, database and session IDs. */
Expand Down
21 changes: 21 additions & 0 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionClientTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.mock;
Expand All @@ -30,6 +31,7 @@
import com.google.cloud.grpc.GrpcTransportOptions;
import com.google.cloud.grpc.GrpcTransportOptions.ExecutorFactory;
import com.google.cloud.spanner.SessionClient.SessionConsumer;
import com.google.cloud.spanner.SessionClient.SessionId;
import com.google.cloud.spanner.spi.v1.SpannerRpc;
import com.google.cloud.spanner.spi.v1.SpannerRpc.Option;
import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -503,4 +505,23 @@ public void onSessionCreateFailure(Throwable t, int createFailureForSessionCount
}
assertThat(returnedSessionCount.get()).isEqualTo(numSessions);
}

@SuppressWarnings("unchecked")
@Test
public void testSessionNamePatterns() {
// Valid pattern for host session name
String host =
"projects/spanner-project/instances/spanner-instance/databases/test-db/sessions/abcd1234";
// Valid pattern for external host session name
String externalHost = "instances/default/databases/test-db/sessions/abcd1234";
try {
SessionId hostSession = SessionId.of(host);
assertEquals("abcd1234", hostSession.getName());
SessionId externalHostSession = SessionId.of(externalHost);
assertEquals("abcd1234", externalHostSession.getName());
// If no exceptions are thrown, the test will pass
Copy link

@krishnamoorthy-r krishnamoorthy-r Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sagnghos sagnghos Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added assert for session name.

} catch (IllegalArgumentException e) {
Copy link
Collaborator

@olavloite olavloite Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fail("Expected no exception to be thrown, but got: " + e.getMessage());
}
}
}
Loading