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 3 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
38 changes: 22 additions & 16 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 @@ -95,31 +97,35 @@ public ChannelEndpoint findServer(BeginTransactionRequest.Builder reqBuilder) {
if (!reqBuilder.hasMutationKey()) {
return null;
}
TargetRange target = recipeCache.mutationToTargetRange(reqBuilder.getMutationKey());
if (target == null) {
return routeMutation(
reqBuilder.getMutationKey(),
preferLeader(reqBuilder.getOptions()),
reqBuilder.getRoutingHintBuilder());
}

public ChannelEndpoint fillRoutingHint(CommitRequest.Builder reqBuilder) {
if (reqBuilder.getMutationsCount() == 0) {
return null;
}
RoutingHint.Builder hintBuilder = RoutingHint.newBuilder();
hintBuilder.setKey(target.start);
if (!target.limit.isEmpty()) {
hintBuilder.setLimitKey(target.limit);
Mutation mutation = reqBuilder.getMutations(0);
return routeMutation(mutation, /* preferLeader= */ true, reqBuilder.getRoutingHintBuilder());
}

private ChannelEndpoint routeMutation(
Mutation mutation, boolean preferLeader, RoutingHint.Builder hintBuilder) {
recipeCache.applySchemaGeneration(hintBuilder);
TargetRange target = recipeCache.mutationToTargetRange(mutation);
if (target == null) {
return null;
}
recipeCache.applyTargetRange(hintBuilder, target);
return fillRoutingHint(
preferLeader(reqBuilder.getOptions()),
preferLeader,
KeyRangeCache.RangeMode.COVERING_SPLIT,
DirectedReadOptions.getDefaultInstance(),
hintBuilder);
}

private ChannelEndpoint fillRoutingHint(
TransactionSelector transactionSelector,
DirectedReadOptions directedReadOptions,
KeyRangeCache.RangeMode rangeMode,
RoutingHint.Builder hintBuilder) {
return fillRoutingHint(
preferLeader(transactionSelector), rangeMode, directedReadOptions, hintBuilder);
}

private ChannelEndpoint fillRoutingHint(
boolean preferLeader,
KeyRangeCache.RangeMode rangeMode,
Expand Down
33 changes: 29 additions & 4 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 @@ -368,10 +372,23 @@ public void sendMessage(RequestT message) {
message = (RequestT) reqBuilder.build();
} else if (message instanceof CommitRequest) {
CommitRequest request = (CommitRequest) message;
String databaseId = parentChannel.extractDatabaseIdFromSession(request.getSession());
if (databaseId != null) {
finder = parentChannel.getOrCreateChannelFinder(databaseId);
}
CommitRequest.Builder reqBuilder = null;
if (finder != null && request.getMutationsCount() > 0) {
reqBuilder = request.toBuilder();
endpoint = finder.fillRoutingHint(reqBuilder);
request = reqBuilder.build();
}
if (!request.getTransactionId().isEmpty()) {
endpoint = parentChannel.affinityEndpoint(request.getTransactionId());
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 overrides the endpoint that was filled above, even in the case if parentChannel.affinityEndpoint(..) returns null, which means that it will fall back to the default endpoint. Is that intentional? (I'm not sure if it can happen in real life, though....)

Copy link
Copy Markdown
Contributor Author

@rahul2393 rahul2393 Mar 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

In case when transactionID is present but no affinity, we should just forward it to the default channel, example

For ExecuteSql / Read, if the first statement falls back to the default endpoint, we do not record default-host affinity because allowDefaultAffinity is false on that path. That means a later CommitRequest can carry a valid transactionId while affinityEndpoint still returns null, in this case we should just forward to default host.

transactionIdToClear = request.getTransactionId();
}
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 != null) {
message = (RequestT) request;
}
} else if (message instanceof RollbackRequest) {
RollbackRequest request = (RollbackRequest) message;
if (!request.getTransactionId().isEmpty()) {
Expand Down Expand Up @@ -610,7 +627,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
31 changes: 17 additions & 14 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 @@ -158,9 +158,7 @@ public void computeKeys(ReadRequest.Builder reqBuilder) {
long reqFp = fingerprint(reqBuilder.buildPartial());

RoutingHint.Builder hintBuilder = reqBuilder.getRoutingHintBuilder();
if (!schemaGeneration.isEmpty()) {
hintBuilder.setSchemaGeneration(schemaGeneration);
}
applySchemaGeneration(hintBuilder);

PreparedRead preparedRead = getIfPresent(preparedReads, reqFp);
if (preparedRead == null) {
Expand All @@ -186,10 +184,7 @@ public void computeKeys(ReadRequest.Builder reqBuilder) {

try {
TargetRange target = recipe.keySetToTargetRange(reqBuilder.getKeySet());
hintBuilder.setKey(target.start);
if (!target.limit.isEmpty()) {
hintBuilder.setLimitKey(target.limit);
}
applyTargetRange(hintBuilder, target);
} catch (IllegalArgumentException e) {
logger.fine("Failed key encoding: " + e.getMessage());
}
Expand All @@ -199,9 +194,7 @@ public void computeKeys(ExecuteSqlRequest.Builder reqBuilder) {
long reqFp = fingerprint(reqBuilder.buildPartial());

RoutingHint.Builder hintBuilder = reqBuilder.getRoutingHintBuilder();
if (!schemaGeneration.isEmpty()) {
hintBuilder.setSchemaGeneration(schemaGeneration);
}
applySchemaGeneration(hintBuilder);

PreparedQuery preparedQuery = getIfPresent(preparedQueries, reqFp);
if (preparedQuery == null) {
Expand All @@ -221,15 +214,25 @@ public void computeKeys(ExecuteSqlRequest.Builder reqBuilder) {

try {
TargetRange target = recipe.queryParamsToTargetRange(reqBuilder.getParams());
hintBuilder.setKey(target.start);
if (!target.limit.isEmpty()) {
hintBuilder.setLimitKey(target.limit);
}
applyTargetRange(hintBuilder, target);
} catch (IllegalArgumentException e) {
logger.fine("Failed query param encoding: " + e.getMessage());
}
}

void applySchemaGeneration(RoutingHint.Builder hintBuilder) {
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?

}
}

void applyTargetRange(RoutingHint.Builder hintBuilder, TargetRange target) {
hintBuilder.setKey(target.start);
if (!target.limit.isEmpty()) {
hintBuilder.setLimitKey(target.limit);
}
}

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