-
Notifications
You must be signed in to change notification settings - Fork 124
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.
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 #2214
Conversation
🤖 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 -- conventional-commit-lint bot |
x-goog-spanner-route-to-leader
header to Spanner RPC contexts f…x-goog-spanner-route-to-leader
header to Spanner RPC contexts f…
…exts for RW/PDML transactions. The header is added to support leader-aware-routing feature, which aims at reducing cross-regional latency for RW/PDML transactions in a multi-region instance.
…re routing feature
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/PartitionedDmlTransactionTest.java
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java
Outdated
Show resolved
Hide resolved
...cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/SpannerMetadataProviderTest.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a couple of nits.
Tip: You could add an extra verification to this test to verify that the header is really sent to Cloud Spanner:
java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/ChannelUsageTest.java
Line 133 in d44e468
// Verify that the compressor name header is set. |
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Outdated
Show resolved
Hide resolved
@@ -275,7 +275,7 @@ public AsyncTransactionManagerImpl transactionManagerAsync(TransactionOption... | |||
@Override | |||
public void prepareReadWriteTransaction() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not relevant for this PR, but note to self/other readers: This seems like a method that can be cleaned up. I don't think it is in use anymore. @rajatbhatta Would you mind taking a look at that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll take a look. @yifanzyifanz: Please keep this comment unresolved for the time being.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerMetadataProvider.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerMetadataProvider.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (apart from one of my pending comments and nits from @olavloite)
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
Outdated
Show resolved
Hide resolved
@@ -275,7 +275,7 @@ public AsyncTransactionManagerImpl transactionManagerAsync(TransactionOption... | |||
@Override | |||
public void prepareReadWriteTransaction() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll take a look. @yifanzyifanz: Please keep this comment unresolved for the time being.
…eader header exists for RW transactions and does not exist for RO transactions or when the leader aware routing feature is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding "Request changes" to prevent accidental merge of this PR. Note that the changes still look good to me.
x-goog-spanner-route-to-leader
header to Spanner RPC contexts f…
Changing the Title to make it consistent across clients. |
Add
x-goog-spanner-route-to-leader
header to Spanner RPC contexts for RW/PDML transactions.The header is added to support leader-aware-routing feature, which aims at reducing cross-regional latency for RW/PDML transactions in a multi-region instance.
This feature is off by default and can be enabled by enabling Leader Aware Routing while creating Spanner object.
Spanner spanner = SpannerOptions.newBuilder().enableLeaderAwareRouting().build().getService();