chore(spanner): implement server connection pool for location-aware routing - #6236
chore(spanner): implement server connection pool for location-aware routing#6236olavloite wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces location-aware routing and endpoint connection pooling for Spanner clients by adding a thread-safe ConnectionCache and a ServerConnection wrapper to track health states and active requests. The reviewer provided several key optimization suggestions: making ActiveRequestGuard 'static by owning ServerConnection to better support async contexts, replacing the lock-free CAS loop with a simpler fetch_sub to avoid CPU contention, using proper Acquire/Release memory ordering for health state transitions, and utilizing tokio::sync::OnceCell in the connection cache to prevent concurrent cache stampedes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6236 +/- ##
==========================================
+ Coverage 96.06% 96.07% +0.01%
==========================================
Files 269 271 +2
Lines 68394 68764 +370
==========================================
+ Hits 65700 66067 +367
- Misses 2694 2697 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1a72834 to
bf08b60
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces thread-safe connection caching and server connection tracking for Spanner location-aware routing. It adds a ConnectionCache to manage server connections and a ServerConnection wrapper to track health states and active inflight requests. The review feedback highlights a critical underflow risk in decrement_active_requests where decrementing a zero count wraps around to usize::MAX, suggesting a lock-free saturating subtraction instead, along with a corresponding test case to verify underflow protection.
bf08b60 to
50375aa
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces location-aware routing and endpoint connection pooling for Spanner clients by adding a thread-safe ConnectionCache and a ServerConnection wrapper to track active inflight requests. The review feedback suggests optimizing ActiveRequestGuard to only hold an Arc<AtomicUsize> reference to avoid expensive clones of ServerConnection (which contains a String and a Channel). Additionally, it is recommended to check the read lock first in ConnectionCache::get before acquiring a write lock to minimize lock contention under high concurrency.
50375aa to
ca07b61
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new routing module in the Spanner client, adding a thread-safe ConnectionCache and a ServerConnection wrapper to support location-aware routing and endpoint connection pooling. Feedback on these changes highlights a compilation issue where OnceCell::initialized() is called but does not exist, suggesting cell.get().is_some() as a replacement. Additionally, reviewers recommended optimizing the ConnectionCache::clear method using retain to avoid unnecessary allocations, and improving performance on the hot path by relaxing atomic ordering from SeqCst to Relaxed for tracking active requests.
ca07b61 to
556fb24
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces location-aware routing and connection pooling capabilities to the Spanner client by adding the routing::connection_cache and routing::server_connection modules, along with comprehensive unit tests. Feedback focuses on optimizing lock acquisition and reducing string allocations in ConnectionCache::get, as well as replacing the CAS loop in ActiveRequestGuard::drop with a simpler fetch_sub operation to reduce concurrency overhead.
556fb24 to
ffe6d66
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces location-aware routing and connection caching for the Spanner client, adding the routing, connection_cache, and server_connection modules. The feedback suggests two performance optimizations: wrapping the inner state of ServerConnection in a single Arc to reduce allocation and cloning overhead on the hot path, and refactoring ConnectionCache::is_empty to use .any() instead of self.len() == 0 to enable short-circuiting.
ffe6d66 to
89d31da
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a thread-safe connection cache and server connection wrapper to support location-aware routing in the Spanner client. Feedback was provided on ActiveRequestGuard::drop to use fetch_update with saturating_sub instead of fetch_sub to prevent potential underflow of the active request counter.
89d31da to
a25e78d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a thread-safe ConnectionCache and ServerConnection wrapper to support location-aware routing and endpoint connection pooling for Spanner clients. Feedback was provided on ConnectionCache::get to avoid an unnecessary String clone by taking the endpoint out of the configuration after creating the channel.
a25e78d to
4bc3f3b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new location-aware routing module for Spanner clients, adding a thread-safe ConnectionCache and a ServerConnection wrapper to track connection health and active requests. The feedback suggests simplifying the connection initialization logic in ConnectionCache::get by cloning the address string upfront instead of using take().unwrap_or_default(), which avoids unnecessary complexity and potential error swallowing in line with the style guide.
4bc3f3b to
1995010
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces location-aware routing components for Spanner, adding ServerConnection to track health and active requests, and ConnectionCache to manage these connections thread-safely. The reviewer suggested a more idiomatic initialization of the default connection cell in ConnectionCache using OnceCell::from.
…outing Add `ServerConnection` and `ConnectionCache` manage server connections and inflight request metrics for location-aware routing.
1995010 to
2914b1f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces location-aware routing modules (routing, connection_cache, and server_connection) to the Spanner client. This includes ServerConnection for wrapping gRPC channels and tracking active requests/health states, and ConnectionCache for thread-safe connection pooling. Feedback highlights that a concurrent cache test performs real network I/O by calling Channel::create, which can lead to slow and flaky tests, and suggests mocking the channel creation for unit tests.
Add
ServerConnectionandConnectionCachemanage server connections and inflight request metrics for location-aware routing.