Skip to content
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

feat: Leader Aware Routing in Connection API #2308

Merged
merged 9 commits into from
Aug 4, 2023
Merged

feat: Leader Aware Routing in Connection API #2308

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f8c63b1
feat: Leader Aware Routing in Connection API
rajatbhatta Feb 27, 2023
42f64b5
Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/co…
rajatbhatta Mar 1, 2023
c3d22c9
add verbose documentation
rajatbhatta Mar 1, 2023
1938945
address review comments
rajatbhatta Mar 1, 2023
e7840fc
change assertThat to assertEquals/assertNotEquals within testGetSpann…
rajatbhatta Mar 6, 2023
b886651
Merge branch 'main' into lar-connection-api
rajatbhatta Jul 21, 2023
12af533
Merge branch 'googleapis:main' into lar-connection-api
rajatbhatta Jul 26, 2023
c709781
add credentials url
rajatbhatta Jul 26, 2023
35a5176
add credentials url
rajatbhatta Jul 26, 2023
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
2 changes: 1 addition & 1 deletion ...le-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ private boolean isValidUri(String uri) {
* database on the emulator will remain untouched. No other configuration is needed in
* order to connect to the emulator than setting this property.
* <li>routeToLeader (boolean): Sets the routeToLeader flag to route requests to leader (true)
* or any region (false) in RW/PDML transactions. Default is true.
* or any region (false) in read/write transactions and Partitioned DML. Default is true.
* </ul>
*
* @param uri The URI of the Spanner database to connect to.
Expand Down
19 changes: 10 additions & 9 deletions ...loud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ public class ConnectionOptionsTest {
private static final String FILE_TEST_PATH =
ConnectionOptionsTest.class.getResource("test-key.json").getFile();
private static final String DEFAULT_HOST = "https://spanner.googleapis.com";
private static final String TEST_PROJECT = "test-project-123";
private static final String TEST_INSTANCE = "test-instance-123";
private static final String TEST_DATABASE = "test-database-123";

@Test
public void testBuildWithURIWithDots() {
Expand Down Expand Up @@ -151,21 +154,19 @@ public void testBuildWithAutoConfigEmulator() {

@Test
public void testBuildWithRouteToLeader() {
final String BASE_URI =
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can be defined as private static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are several places in this file where this can be done and replace the hard-codings with constants. We can take it as a follow-up task in a separate PR.

"cloudspanner:/projects/test-project-123/instances/test-instance-123/databases/test-database-123";
ConnectionOptions.Builder builder = ConnectionOptions.newBuilder();
builder.setUri(
"cloudspanner:/projects/test-project-123/instances/test-instance-123/databases/test-database-123?routeToLeader=false");
builder.setUri(BASE_URI + "?routeToLeader=false");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Prefer StringBuilder() . + is a more expensive way of concatenating strings.

Copy link
Contributor Author

@rajatbhatta rajatbhatta Jul 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think for single statement, the compiler would use StringBuilder automatically: https://stackoverflow.com/a/4645155.

ConnectionOptions options = builder.build();
assertEquals(options.getHost(), DEFAULT_HOST);
Copy link
Contributor

Choose a reason for hiding this comment

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

While the asserts here are consistent with what is used in the test class, we should try having a single assert statement. For ex - assertEquals(expectedConnectionOptions, actualConnectionOptions)

This provides benefits where you asserting the entire object and not picking few of the members and can help to pick code side-effects where any member got modified by mistake.

Copy link
Contributor Author

@rajatbhatta rajatbhatta Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I understand your concern here. I'll want to take this up as a separate effort, as it's changing the way we have been writing unit tests so far. Let's discuss more in details offline, and take it forward from there. Keeping this thread open until then.

assertEquals(options.getProjectId(), "test-project-123");
assertEquals(options.getInstanceId(), "test-instance-123");
assertEquals(options.getDatabaseName(), "test-database-123");
assertEquals(options.getProjectId(), TEST_PROJECT);
assertEquals(options.getInstanceId(), TEST_INSTANCE);
assertEquals(options.getDatabaseName(), TEST_DATABASE);
assertFalse(options.isRouteToLeader());

// Test for default behavior for routeToLeader property.
builder =
ConnectionOptions.newBuilder()
.setUri(
"cloudspanner:/projects/test-project-123/instances/test-instance-123/databases/test-database-123");
builder = ConnectionOptions.newBuilder().setUri(BASE_URI);
options = builder.build();
assertTrue(options.isRouteToLeader());
}
Expand Down
25 changes: 13 additions & 12 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SpannerPoolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,40 +117,40 @@ public void testGetSpanner() {
// assert equal
spanner1 = pool.getSpanner(options1, connection1);
spanner2 = pool.getSpanner(options1, connection2);
assertThat(spanner1).isEqualTo(spanner2);
assertEquals(spanner1, spanner2);
spanner1 = pool.getSpanner(options2, connection1);
spanner2 = pool.getSpanner(options2, connection2);
assertThat(spanner1).isEqualTo(spanner2);
assertEquals(spanner1, spanner2);
spanner1 = pool.getSpanner(options3, connection1);
spanner2 = pool.getSpanner(options3, connection2);
assertThat(spanner1).isEqualTo(spanner2);
assertEquals(spanner1, spanner2);
spanner1 = pool.getSpanner(options4, connection1);
spanner2 = pool.getSpanner(options4, connection2);
assertThat(spanner1).isEqualTo(spanner2);
assertEquals(spanner1, spanner2);
// Options 5 and 6 both use default credentials.
spanner1 = pool.getSpanner(options5, connection1);
spanner2 = pool.getSpanner(options6, connection2);
assertThat(spanner1).isEqualTo(spanner2);
assertEquals(spanner1, spanner2);

// assert not equal
spanner1 = pool.getSpanner(options1, connection1);
spanner2 = pool.getSpanner(options2, connection2);
assertThat(spanner1).isNotEqualTo(spanner2);
assertNotEquals(spanner1, spanner2);
spanner1 = pool.getSpanner(options1, connection1);
spanner2 = pool.getSpanner(options3, connection2);
assertThat(spanner1).isNotEqualTo(spanner2);
assertNotEquals(spanner1, spanner2);
spanner1 = pool.getSpanner(options1, connection1);
spanner2 = pool.getSpanner(options4, connection2);
assertThat(spanner1).isNotEqualTo(spanner2);
assertNotEquals(spanner1, spanner2);
spanner1 = pool.getSpanner(options2, connection1);
spanner2 = pool.getSpanner(options3, connection2);
assertThat(spanner1).isNotEqualTo(spanner2);
assertNotEquals(spanner1, spanner2);
spanner1 = pool.getSpanner(options2, connection1);
spanner2 = pool.getSpanner(options4, connection2);
assertThat(spanner1).isNotEqualTo(spanner2);
assertNotEquals(spanner1, spanner2);
spanner1 = pool.getSpanner(options3, connection1);
spanner2 = pool.getSpanner(options4, connection2);
assertThat(spanner1).isNotEqualTo(spanner2);
assertNotEquals(spanner1, spanner2);
spanner1 = pool.getSpanner(options7, connection1);
surbhigarg92 marked this conversation as resolved.
Show resolved Hide resolved
spanner2 = pool.getSpanner(options8, connection2);
assertNotEquals(spanner1, spanner2);
rajatbhatta marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -469,6 +469,8 @@ public void testSpannerPoolKeyEquality() {
.setUri("cloudspanner:/projects/p/instances/i/databases/d")
.setCredentials(NoCredentials.getInstance())
.build();
// Not passing in routeToLeader in Connection URI is equivalent to passing it as true,
// as routeToLeader is true by default.
ConnectionOptions options4 =
ConnectionOptions.newBuilder()
.setUri("cloudspanner:/projects/p/instances/i/databases/d?routeToLeader=true")
Expand All @@ -492,6 +494,5 @@ public void testSpannerPoolKeyEquality() {
assertNotEquals(key1, new Object());
assertEquals(key3, key4);
assertNotEquals(key4, key5);
assertEquals(key2, key4);
}
}