Skip to content
This repository was archived by the owner on Apr 7, 2026. It is now read-only.

chore: add routing hints and cache updates for begin/commit #4392

Merged
rahul2393 merged 4 commits into from
Mar 25, 2026
Merged

chore: add routing hints and cache updates for begin/commit #4392

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
82c3244
chore(spanner): add routing hints and cache updates for begin/commit
rahul2393 Mar 25, 2026
3c99d2a
chore: generate libraries at Wed Mar 25 07:01:43 UTC 2026
cloud-java-bot Mar 25, 2026
e836a5d
incorporate suggestions
rahul2393 Mar 25, 2026
0f39595
use same heuristic for selecting mutation for commit routing which we…
rahul2393 Mar 25, 2026
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
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ If you are using Maven with [BOM][libraries-bom], add this to your pom.xml file:
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>libraries-bom</artifactId>
<version>26.76.0</version>
<version>26.78.0</version>
<type>pom</type>
<scope>import</scope>
</dependency>
Expand All @@ -41,7 +41,7 @@ If you are using Maven without the BOM, add this to your dependencies:
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-spanner</artifactId>
<version>6.110.0</version>
<version>6.112.0</version>
</dependency>

```
Expand Down
32 changes: 21 additions & 11 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/ChannelFinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@
import com.google.api.core.InternalApi;
import com.google.spanner.v1.BeginTransactionRequest;
import com.google.spanner.v1.CacheUpdate;
import com.google.spanner.v1.CommitRequest;
import com.google.spanner.v1.DirectedReadOptions;
import com.google.spanner.v1.ExecuteSqlRequest;
import com.google.spanner.v1.Mutation;
import com.google.spanner.v1.ReadRequest;
import com.google.spanner.v1.RoutingHint;
import com.google.spanner.v1.TransactionOptions;
Expand Down Expand Up @@ -92,23 +94,31 @@ public ChannelEndpoint findServer(ExecuteSqlRequest.Builder reqBuilder, boolean
}

public ChannelEndpoint findServer(BeginTransactionRequest.Builder reqBuilder) {
if (!reqBuilder.hasMutationKey()) {
if (!reqBuilder.hasMutationKey()
|| !recipeCache.computeKeys(
Copy link
Copy Markdown
Collaborator

@olavloite olavloite Mar 25, 2026

Choose a reason for hiding this comment

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

I find some of this code very hard to read. E.g. the fact that computeKeys(..) returns a boolean and has the side-effect that it modifies the RoutingHintBuilder that it gets (but only if it returns true, but also sometimes when it returns false, see also below) makes this very hard to understand. I think it would be good if we would take a 'readability sweep' of both this method, but also the code in general.

reqBuilder.getMutationKey(), reqBuilder.getRoutingHintBuilder())) {
return null;
}
TargetRange target = recipeCache.mutationToTargetRange(reqBuilder.getMutationKey());
if (target == null) {
return null;
}
RoutingHint.Builder hintBuilder = RoutingHint.newBuilder();
hintBuilder.setKey(target.start);
if (!target.limit.isEmpty()) {
hintBuilder.setLimitKey(target.limit);
}
return fillRoutingHint(
preferLeader(reqBuilder.getOptions()),
KeyRangeCache.RangeMode.COVERING_SPLIT,
DirectedReadOptions.getDefaultInstance(),
hintBuilder);
reqBuilder.getRoutingHintBuilder());
}

public void fillRoutingHint(CommitRequest.Builder reqBuilder) {
if (reqBuilder.getMutationsCount() == 0) {
return;
}
Mutation mutation = reqBuilder.getMutations(0);
if (!recipeCache.computeKeys(mutation, reqBuilder.getRoutingHintBuilder())) {
return;
}
fillRoutingHint(
/* preferLeader= */ true,
KeyRangeCache.RangeMode.COVERING_SPLIT,
DirectedReadOptions.getDefaultInstance(),
reqBuilder.getRoutingHintBuilder());
}

private ChannelEndpoint fillRoutingHint(
Copy link
Copy Markdown
Collaborator

@olavloite olavloite Mar 25, 2026

Choose a reason for hiding this comment

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

Maybe not directly related to this change, but I noticed that this method is unused. Can we remove it?

Expand Down
36 changes: 28 additions & 8 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyAwareChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.protobuf.ByteString;
import com.google.spanner.v1.BeginTransactionRequest;
import com.google.spanner.v1.CommitRequest;
import com.google.spanner.v1.CommitResponse;
import com.google.spanner.v1.ExecuteSqlRequest;
import com.google.spanner.v1.PartialResultSet;
import com.google.spanner.v1.ReadRequest;
Expand All @@ -47,9 +48,10 @@
/**
* ManagedChannel that routes eligible requests using location-aware routing hints.
*
* <p>Routing hints are applied to streaming read/query and unary ExecuteSql. Commit/Rollback use
* transaction affinity when available. BeginTransaction is routed only when a mutation key is
* provided.
* <p>Routing hints are applied to streaming read/query and unary ExecuteSql. Mutation-based
* BeginTransaction and Commit requests also carry routing hints when recipes are available.
* Commit/Rollback use transaction affinity when available. BeginTransaction is routed only when a
* mutation key is provided.
*/
@InternalApi
final class KeyAwareChannel extends ManagedChannel {
Expand Down Expand Up @@ -355,8 +357,10 @@ public void sendMessage(RequestT message) {
BeginTransactionRequest.Builder reqBuilder =
((BeginTransactionRequest) message).toBuilder();
String databaseId = parentChannel.extractDatabaseIdFromSession(reqBuilder.getSession());
if (databaseId != null && reqBuilder.hasMutationKey()) {
if (databaseId != null) {
finder = parentChannel.getOrCreateChannelFinder(databaseId);
}
if (finder != null && reqBuilder.hasMutationKey()) {
endpoint = finder.findServer(reqBuilder);
}
if (reqBuilder.hasOptions() && reqBuilder.getOptions().hasReadOnly()) {
Expand All @@ -367,11 +371,19 @@ public void sendMessage(RequestT message) {
}
message = (RequestT) reqBuilder.build();
} else if (message instanceof CommitRequest) {
CommitRequest request = (CommitRequest) message;
if (!request.getTransactionId().isEmpty()) {
endpoint = parentChannel.affinityEndpoint(request.getTransactionId());
transactionIdToClear = request.getTransactionId();
CommitRequest.Builder reqBuilder = ((CommitRequest) message).toBuilder();
Copy link
Copy Markdown
Collaborator

@olavloite olavloite Mar 25, 2026

Choose a reason for hiding this comment

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

This could potentially be a very heavy operation, as a CommitRequest could contain a very large number of mutations (e.g. during a bulk load). Converting it back to a builder means copying all of those mutations. I think that we should only call toBuilder() if we really know that we need the builder (i.e. if we know that we will be setting a routing hint).

This is also true for the other types of requests here, but those are less likely to be as heavy as a CommitRequest.

String databaseId = parentChannel.extractDatabaseIdFromSession(reqBuilder.getSession());
if (databaseId != null) {
finder = parentChannel.getOrCreateChannelFinder(databaseId);
}
if (finder != null) {
finder.fillRoutingHint(reqBuilder);
}
Comment on lines +376 to +392
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 25, 2026

Choose a reason for hiding this comment

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

medium

Since getOrCreateChannelFinder never returns null, the null check for finder is redundant. You can combine these two if statements for better readability and conciseness.

          if (databaseId != null) {
            finder = parentChannel.getOrCreateChannelFinder(databaseId);
            finder.fillRoutingHint(reqBuilder);
          }

if (!reqBuilder.getTransactionId().isEmpty()) {
Copy link
Copy Markdown
Collaborator

@olavloite olavloite Mar 25, 2026

Choose a reason for hiding this comment

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

TransactionId is empty if the CommitRequest has a single-use read/write transaction. Meaning that we skip this for 'write-at-least-once' transactions. Is that intentional? Normally, such a CommitRequest would contain mutations, meaning that we would be able to route in using the same logic as a BeginTransactionRequest that contains a mutation key, right?

endpoint = parentChannel.affinityEndpoint(reqBuilder.getTransactionId());
transactionIdToClear = reqBuilder.getTransactionId();
}
message = (RequestT) reqBuilder.build();
} else if (message instanceof RollbackRequest) {
RollbackRequest request = (RollbackRequest) message;
if (!request.getTransactionId().isEmpty()) {
Expand Down Expand Up @@ -610,7 +622,15 @@ public void onMessage(ResponseT message) {
transactionId = transactionIdFromMetadata(response);
} else if (message instanceof Transaction) {
Transaction response = (Transaction) message;
if (response.hasCacheUpdate() && call.channelFinder != null) {
call.channelFinder.update(response.getCacheUpdate());
}
transactionId = transactionIdFromTransaction(response);
} else if (message instanceof CommitResponse) {
CommitResponse response = (CommitResponse) message;
if (response.hasCacheUpdate() && call.channelFinder != null) {
call.channelFinder.update(response.getCacheUpdate());
}
}
if (transactionId != null) {
if (call.isReadOnlyBegin) {
Expand Down
17 changes: 17 additions & 0 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/KeyRecipeCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,23 @@ public void computeKeys(ExecuteSqlRequest.Builder reqBuilder) {
}
}

boolean computeKeys(Mutation mutation, RoutingHint.Builder hintBuilder) {
Copy link
Copy Markdown
Collaborator

@olavloite olavloite Mar 25, 2026

Choose a reason for hiding this comment

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

This method in general is quite hard to read, as it both returns a value that indicates whether the method did something or not, and has a potential side-effect of (maybe) populating the hintBuilder. Is there a better way to do this?

Copy link
Copy Markdown
Contributor Author

@rahul2393 rahul2393 Mar 25, 2026

Choose a reason for hiding this comment

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

removed that helper from the mutation routing flow.

if (!schemaGeneration.isEmpty()) {
hintBuilder.setSchemaGeneration(schemaGeneration);
Copy link
Copy Markdown
Collaborator

@olavloite olavloite Mar 25, 2026

Choose a reason for hiding this comment

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

This updates the hint builder, but the method might still return false if the TargetRange below is null. That seems weird. I would expect this method only to modify the hint builder if it also returns true. Can we otherwise at least add some documentation to the method that explains when it returns true/false, and when it modifies the hint builder?

}

TargetRange target = mutationToTargetRange(mutation);
if (target == null) {
return false;
}

hintBuilder.setKey(target.start);
if (!target.limit.isEmpty()) {
hintBuilder.setLimitKey(target.limit);
}
return true;
}

public TargetRange mutationToTargetRange(Mutation mutation) {
if (mutation == null) {
return null;
Expand Down
Loading
Loading