Skip to content

chore(spanner): implement dynamic channel pooling and transaction affinity - #6728

Draft
olavloite wants to merge 4 commits into
googleapis:mainfrom
olavloite:spanner-dynamic-channel-pool-and-affinity
Draft

chore(spanner): implement dynamic channel pooling and transaction affinity#6728
olavloite wants to merge 4 commits into
googleapis:mainfrom
olavloite:spanner-dynamic-channel-pool-and-affinity

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

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.

@olavloite
olavloite requested review from a team as code owners September 7, 2026 17:23
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Sep 7, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/spanner/src/batch_read_only_transaction.rs
Comment thread src/spanner/src/batch_read_only_transaction.rs
Comment thread src/spanner/src/channel_pool/pool.rs
Comment thread src/spanner/src/client.rs
@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.99%. Comparing base (4afc819) to head (98920dc).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6728      +/-   ##
==========================================
+ Coverage   96.95%   96.99%   +0.04%     
==========================================
  Files         313      313              
  Lines      105118   105957     +839     
==========================================
+ Hits       101916   102778     +862     
+ Misses       3202     3179      -23     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@olavloite
olavloite force-pushed the spanner-dynamic-channel-pool-and-affinity branch from 4db3c64 to febb3bb Compare September 7, 2026 17:38
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/spanner/src/batch_read_only_transaction.rs
Comment thread src/spanner/src/batch_read_only_transaction.rs
Comment thread src/spanner/src/client.rs
…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.
@olavloite
olavloite force-pushed the spanner-dynamic-channel-pool-and-affinity branch from febb3bb to d82af44 Compare September 8, 2026 06:11
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/spanner/src/client.rs
Comment thread src/spanner/src/client.rs Outdated
Comment thread src/spanner/src/batch_read_only_transaction.rs
Comment thread src/spanner/src/batch_read_only_transaction.rs
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/spanner/src/client.rs
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request integrates a unified ChannelPool into the Spanner client, replacing the previous round-robin channel selection with support for static and dynamic channel pool configurations, transaction affinity pinning, and transport error penalization. The review feedback highlights a critical correctness bug in resolve_cas_conflict where stale active candidates can cause transaction affinity loss in Read/Write transactions. Additionally, high-severity retry bugs were identified in execute_query and execute_read within batch_read_only_transaction.rs because creating the builder outside the retry closure prevents successful retries, along with corresponding opportunities to clean up unused actual_gax_options.

Comment thread src/spanner/src/channel_pool/pool.rs
Comment thread src/spanner/src/batch_read_only_transaction.rs
Comment thread src/spanner/src/batch_read_only_transaction.rs
Comment thread src/spanner/src/batch_read_only_transaction.rs
Comment thread src/spanner/src/batch_read_only_transaction.rs
@olavloite
olavloite force-pushed the spanner-dynamic-channel-pool-and-affinity branch from f3f51c3 to 75d7f79 Compare September 8, 2026 13:09
@olavloite
olavloite force-pushed the spanner-dynamic-channel-pool-and-affinity branch from 75d7f79 to 98920dc Compare September 8, 2026 13:20
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request integrates a unified ChannelPool into the Spanner client, replacing the previous round-robin channel selection and manual channel_hint tracking with robust static and dynamic channel pooling, load-balanced selection (P2C), and transaction affinity pinning via TransactionAffinity. It also introduces RAII stream lifetime guards to track RPC results and apply error penalties for dynamic scaling. The feedback highlights a critical issue in BatchReadOnlyTransaction where the closures passed to execute_partition_stream are FnOnce because they consume the stream builders, which prevents retries. To support multiple attempts, the builders should be cloned inside the closures.

Comment on lines +411 to +412
Self::execute_partition_stream(client, "ExecuteStreamingSql", move || builder.send())
.await?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

The move || builder.send() closure is an FnOnce because builder.send() consumes builder. If execute_partition_stream is intended to perform retries (which is likely for transient errors or transaction aborts), it would need to call the closure multiple times, which is not possible with an FnOnce.

Since the stream builder (ExecuteStreamingSql) implements Clone, you can capture builder by reference and clone it inside the closure. This makes the closure an FnMut, allowing it to be called multiple times for retries.

Suggested change
Self::execute_partition_stream(client, "ExecuteStreamingSql", move || builder.send())
.await?;
Self::execute_partition_stream(client, "ExecuteStreamingSql", || builder.clone().send())
.await?;

.send()
})
.await?;
Self::execute_partition_stream(client, "StreamingRead", move || builder.send()).await?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

Similar to the execute_sql case, the move || builder.send() closure is an FnOnce, which prevents retries within execute_partition_stream. Since StreamingRead builder is also Clone, you should clone it inside the closure to make it an FnMut and support retries.

Suggested change
Self::execute_partition_stream(client, "StreamingRead", move || builder.send()).await?;
Self::execute_partition_stream(client, "StreamingRead", || builder.clone().send()).await?;

@olavloite
olavloite marked this pull request as draft September 8, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant