chore(spanner): wire location-aware routing into streaming read and query RPCs - #6494
chore(spanner): wire location-aware routing into streaming read and query RPCs#6494olavloite wants to merge 3 commits into
Conversation
…uery RPCs - Add `channel_id` directly to `Channel` to avoid redundant modulo arithmetic when attaching Spanner request ID headers. - Implement protobuf-level key extraction helpers in `key_extractor` (`extract_proto_read_request_routing_key`, `extract_key_from_proto_key_set`) supporting point keys and key ranges (`start_closed`/`start_open`). - Add `resolve_routing_connection` to `DatabaseClient` and wire it into `streaming_read` and `execute_query` RPC dispatch. - Keep location-aware routing disabled by default, gating activation behind the `GOOGLE_SPANNER_EXPERIMENTAL_LOCATION_API` environment variable.
There was a problem hiding this comment.
Code Review
This pull request refactors the Spanner client to pass direct Channel references instead of channel_hint indices, simplifying request ID generation by storing a 1-based channel_id on the Channel struct. It also introduces location-aware routing for streaming reads by extracting routing keys from ReadRequest to resolve optimal server connections. Feedback on these changes includes resolving a missing import for ProtoKeyRange in tests, optimizing resolve_routing_connection to return early when no routing information is present, and removing the now-unused next_channel_hint function.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6494 +/- ##
==========================================
+ Coverage 96.39% 96.41% +0.02%
==========================================
Files 300 300
Lines 84585 85343 +758
==========================================
+ Hits 81533 82283 +750
- Misses 3052 3060 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Spanner client to support location-aware routing by passing Channel references directly to RPC methods instead of numeric channel hints. It introduces connection resolution logic based on transaction IDs and routing keys, and adds comprehensive tests to verify both standard round-robin channel pooling and location-aware routing. Feedback on these changes highlights two key issues: first, use_transaction_affinity is hardcoded to false in RoutingContext, which disables transaction affinity routing even when a transaction ID is present; second, errors during key extraction are silently swallowed with .ok(), violating the repository style guide's rule against swallowing errors.
c2dc845 to
204cf59
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Spanner client's channel selection and request ID attachment by passing direct Channel references instead of numeric hints, storing a 1-based channel_id directly on the Channel struct. It also introduces location-aware routing for Spanner Omni, enabling the DatabaseClient to resolve optimal server connections using transaction affinity and routing keys extracted from read requests, while maintaining standard round-robin channel pooling as a fallback. I have reviewed the changes and have no feedback to provide as there are no review comments and the implementation is robust and well-tested.
channel_iddirectly toChannelto avoid redundant modulo arithmeticwhen attaching Spanner request ID headers.
key_extractor(
extract_proto_read_request_routing_key,extract_key_from_proto_key_set)supporting point keys and key ranges (
start_closed/start_open).resolve_routing_connectiontoDatabaseClientand wire it intostreaming_readandexecute_queryRPC dispatch.GOOGLE_SPANNER_EXPERIMENTAL_LOCATION_APIenvironment variable.