chore(spanner): implement dynamic channel pooling and transaction affinity - #6728
chore(spanner): implement dynamic channel pooling and transaction affinity#6728olavloite wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request integrates a unified ChannelPool into the Spanner client to support dynamic channel pooling, load-aware channel selection, and transaction channel affinity pinning. The feedback identifies several critical issues: in BatchReadOnlyTransaction, creating the query and read builders outside the retry closures prevents proper retry execution and defeats dynamic channel pooling; in ChannelPool::resolve_cas_conflict, ignoring CAS failures when re-pinning closed channels violates transaction affinity for Read/Write transactions; and in Spanner client configuration, retrieving the pool config override fails because extensions are wrapped in Arc.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6728 +/- ##
==========================================
+ Coverage 96.80% 96.85% +0.04%
==========================================
Files 313 313
Lines 104285 105154 +869
==========================================
+ Hits 100958 101848 +890
+ Misses 3327 3306 -21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4db3c64 to
febb3bb
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates a unified ChannelPool into the Spanner client, replacing the old static round-robin channel array with support for both static and dynamic channel pool configurations. It updates transactions and the DatabaseClient to use TransactionAffinity and channel leases instead of raw channel_hint integers, and introduces StreamLifetimeGuards to record RPC error codes for dynamic channel scaling. Feedback on the changes highlights critical bugs: the closures in batch_read_only_transaction.rs consume the builder by value and must clone it to support retries, and the configuration retrieval in client.rs must specify the Arc wrapper to successfully remove the extension.
…inity Adds dynamic channel pooling to the Spanner client, replacing single-channel stubs with an elastic pool of gRPC connections. - Distributes requests across channels using the Power of Two Choices (P2C) algorithm to avoid hot connections. - Automatically spins up and warms new channels under heavy load, and quietly drains idle ones when traffic drops. - Pins multi-step transactions to the same channel so server state remains consistent. - Supports both static and dynamic configurations, and respects the `SPANNER_NUM_CHANNELS` environment variable. - Keeps the pool configuration internal (`pub(crate)`) for now while the API and behavior are finalized.
febb3bb to
d82af44
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates a unified ChannelPool into the Spanner client, replacing the simple round-robin channel selection with a pool that supports static and dynamic scaling, transaction affinity pinning, and error penalization. It updates DatabaseClient RPCs and streaming operations to lease channels and utilize lifetime guards to track active calls. The review feedback highlights a critical type mismatch bug in resolve_pool_config_with where ChannelPoolConfig is retrieved directly instead of as an Arc<ChannelPoolConfig>, which is how the builder stores extensions. Additionally, in BatchReadOnlyTransaction, the closures for streaming retries consume the builder on the first invocation, which will cause failures during retries; the builder should be cloned inside the closures to support multiple attempts.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a dynamic channel pooling mechanism for the Spanner client, replacing the previous static channel list. Key changes include the introduction of ChannelPool, ChannelLease, and TransactionAffinity to manage gRPC channel lifecycles, load-based scaling, and transaction affinity. The Spanner client has been updated to use this pool, and various RPC methods now accept TransactionAffinity or ChannelLease to ensure correct routing and affinity. The code review feedback correctly identified an issue with the use of remove on Extensions, which is not supported by all implementations, and provided an actionable suggestion to use get instead.
Adds dynamic channel pooling to the Spanner client, replacing single-channel stubs with an elastic pool of gRPC connections.
SPANNER_NUM_CHANNELSenvironment variable.pub(crate)) for now while the API and behavior are finalized.