feat: Leader Aware Routing in Connection API#2308
feat: Leader Aware Routing in Connection API#2308rajatbhatta merged 9 commits intogoogleapis:mainfrom
Conversation
product-auto-label
Bot
added
size: m
(cherry picked from commit 83ded36)
rajatbhatta
force-pushed
the
lar-connection-api
branch
from
13cb842 to
f8c63b1
Compare
…nnection/ConnectionOptions.java Co-authored-by: Knut Olav Løite <[email protected]>
| @@ -65,6 +65,8 @@ public class SpannerPoolTest { | |||
|
|
|||
There was a problem hiding this comment.
Query : Do we have a test which fails when the new property is not added to the equals() method?
There was a problem hiding this comment.
As discussed offline, we can take it as a separate exercise, as we'll need this across classes. Keeping the thread open for the time being.
| builder.setUri( | ||
| "cloudspanner:/projects/test-project-123/instances/test-instance-123/databases/test-database-123?routeToLeader=false"); | ||
| ConnectionOptions options = builder.build(); | ||
| assertEquals(options.getHost(), DEFAULT_HOST); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| @Test | ||
| public void testBuildWithRouteToLeader() { | ||
| final String BASE_URI = |
There was a problem hiding this comment.
Nit: Can be defined as private static.
There was a problem hiding this comment.
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.
| 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"); |
There was a problem hiding this comment.
Nit: Prefer StringBuilder() . + is a more expensive way of concatenating strings.
There was a problem hiding this comment.
I think for single statement, the compiler would use StringBuilder automatically: https://stackoverflow.com/a/4645155.
rajatbhatta
added
the
owlbot:run
gcf-owl-bot
Bot
removed
the
owlbot:run
rajatbhatta
added
the
do not merge
|
Pls do not merge this PR until LAR is enabled by default in the Java client library. |
4 participants
By default, RW/PDML requests are preferred to route to leader after LAR is enabled from backend.
This PR adds
routeToLeaderBoolean connection option for implementing opt-out feature for LAR.Example URL:
cloudspanner:/projects/test-project-123/instances/test-instance-123/databases/test-database-123?routeToLeader=falseNote: You can also pass in
routeToLeader=true, but that is the default behaviour (even if you do not pass in this option).BEGIN_COMMIT_OVERRIDE
feat: Enable leader aware routing by default in Connection API. This enables its use in the JDBC driver and PGAdapter. The update contains performance optimisations that will reduce the latency of read/write transactions that originate from a region other than the default leader region.
END_COMMIT_OVERRIDE