chore(spanner): add location-aware routing hint generation and key recipe caching - #6563
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements location-aware routing for the Spanner client, enabling the resolution of optimal server connections and routing hints in a single atomic pass. It introduces schema generation tracking to invalidate stale cache entries and updates the CacheUpdater and KeyRecipeCache to handle these changes efficiently. The review identified a type mismatch in extract_transaction_id and a move-borrow conflict in update_from_recipe_list, both of which require fixes to ensure compilation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6563 +/- ##
==========================================
+ Coverage 96.53% 96.57% +0.03%
==========================================
Files 304 304
Lines 90380 91285 +905
==========================================
+ Hits 87249 88157 +908
+ Misses 3131 3128 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
191fdd8 to
727a93a
Compare
727a93a to
68f5b85
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates location-aware routing hints into Spanner streaming RPCs by resolving the optimal server connection and routing hint in a single atomic pass. It introduces schema generation tracking and invalidation within the key recipe cache, refactors cache updates to be processed by value, and implements unified route resolution in the location router. A review comment correctly identifies an issue in the tablet fallback path where querying eligible tablets with prefer_leader = true after a leader failure prevents routing to healthy follower replicas, suggesting a fallback to false instead.
68f5b85 to
d8ec940
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements location-aware routing hints for Spanner streaming RPCs by introducing a single-pass route resolution mechanism (resolve_route) that atomically determines the target server connection and constructs the corresponding RoutingHint. It also updates KeyRecipeCache to track schema generation versions, invalidating stale cache entries on schema bumps, and refactors CacheUpdater to process updates by value. Additionally, the routing fallback logic is improved to fail over to healthy follower replicas when a leader is on cooldown. Feedback on the changes points out a critical synchronization issue in cache_subscriber.rs where bypassing the update lock when processing cache updates can lead to data races and out-of-sync database IDs.
d8ec940 to
8d178bb
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements location-aware routing enhancements for the Spanner database client, including atomic resolution of server connections and routing hints, improved concurrency in CacheUpdater via RwLock, and schema generation tracking in KeyRecipeCache to manage cache invalidations. The reviewer provided an optimization suggestion for KeyRecipeCache::update_from_recipe_list to reduce lock contention by deferring the deallocation of displaced recipes until after the write lock is released.
…cipe caching - Add `LocationRouter::resolve_route` for unified, single-pass resolution of target `ServerConnection` and `RoutingHint`, eliminating sampling drift and TOCTOU races between transport selection and hint stamping. - Add `KeyRecipeCache` to ingest and store `RecipeList` definitions by schema generation, supporting zero-allocation table/index lookups, CLOCK (Second-Chance) eviction for query recipes, and atomic cache invalidation on schema version bumps. - Update `CacheUpdater::process_cache_update` to ingest owned `CacheUpdate` payloads by value, enabling zero-copy recipe transfer. - Wire single-pass streaming route resolution into `DatabaseClient`, populating `RoutingHint` on streaming read/query requests when location routing is active.
8d178bb to
8df182f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates location-aware request routing and routing hint generation into the Spanner client. It refactors the routing resolution logic to perform a single-pass atomic resolution of both the target ServerConnection and the RoutingHint, ensuring consistency and preventing TOCTOU races. The cache update mechanism is optimized by moving the database ID and update synchronization locks (RwLock) directly into CacheUpdater, allowing concurrent incremental updates under a shared read lock while synchronizing database transitions under an exclusive write lock. Additionally, KeyRecipeCache now tracks schema generations to handle stale updates and invalidate outdated recipes. Comprehensive unit and integration tests have been added to verify these routing, caching, and fallback behaviors. I have no feedback to provide as there are no review comments to evaluate.
LocationRouter::resolve_routefor unified, single-pass resolution of targetServerConnectionandRoutingHint, eliminating sampling drift and TOCTOU races between transport selection and hint stamping.KeyRecipeCacheto ingest and storeRecipeListdefinitions by schema generation, supporting zero-allocation table/index lookups, CLOCK (Second-Chance) eviction for query recipes, and atomic cache invalidation on schema version bumps.CacheUpdater::process_cache_updateto ingest ownedCacheUpdatepayloads by value, enabling zero-copy recipe transfer.DatabaseClient, populatingRoutingHinton streaming read/query requests when location routing is active.