Repository navigation
fix(spanner): keep DCP transaction on its channel while it drains - #20618
Conversation
Add TestDCPReadWriteTransactionChannelAffinityAcrossDrain. A read-write transaction bound to a DCP channel should keep using that channel when it is drained mid-transaction, and move to another active channel only after the channel is closed. The draining case fails on current code because dcpResolvingSpannerClient.resolve re-picks as soon as the entry is no longer active.
There was a problem hiding this comment.
Code Review
This pull request modifies the dynamic channel pool to maintain channel affinity for read-write transactions during scale-down, ensuring operations stay on the same channel until it is closed. It replaces entry ID tracking with an atomic pointer to the dcpEntry and introduces a safe idle-closing mechanism. The review feedback identifies a critical race condition between claimClose and a permanent close() that could lead to resource leaks during pool shutdown. The reviewer suggests introducing a new dcpStateClosedPermanently state and provides code suggestions to ensure permanent closes are respected.
With the dynamic channel pool, a session handle re-resolved its bound channel by id among active entries on every RPC, so a read-write transaction moved to another channel as soon as scale-down marked its channel draining. Keep a pointer to the bound entry instead and use it while it is active or draining; re-pick only once it is closed. Each entry now has a reference count that is negative once the entry is closed. The session handle resolver takes a reference before each RPC and releases it when the RPC returns, and a stream holds its own reference until it finishes. Scale-down drain closes an idle entry only by swapping zero references for closed, so an RPC that acquired the entry keeps its channel open and a later one fails to acquire and re-picks. Pool shutdown closes regardless of references and always wins. Concurrent RPCs of one handle agree on a replacement entry via compare-and-swap.
fa5976a to
e32b6ff
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a reference-counting mechanism to the Spanner dynamic channel pool (dcpEntry) to prevent draining channels from being closed while active RPCs or streams are still using them. It refactors dcpResolvingSpannerClient to track the dcpEntry pointer directly and acquire/release references atomically during RPC execution, ensuring that read-write transactions maintain channel affinity through scale-down events. Extensive unit tests have been added to verify these concurrency behaviors. There are no review comments to address, and the implementation looks solid.
| // closeIfIdle closes a draining entry that has been idle for grace, unless an | ||
| // RPC holds a reference to it. It reports whether the entry is closed. | ||
| func (e *dcpEntry) closeIfIdle(grace time.Duration) bool { | ||
| return e.idleFor(grace) && e.closeIfUnreferenced() |
There was a problem hiding this comment.
If I understand the flow correctly here, then:
- The reference counter for a channel is only increased as long as there is an active RPC on it.
- That means that a draining channel that is in use by a read/write transaction is only prevented from being closed by the grace period when the read/write transaction is not actively executing any RPCs.
- The default grace period is 1 minute, so more than enough for an inactive read/write transaction, as it will be aborted by Spanner after 10s.
- However, the grace period is configurable, meaning that an application could set the grace period to 1s, and thereby (unintentionally) cause a problem for itself.
Should we therefore add a fixed duration of 15s here to ensure that a channel is only closed after 15s + the grace period (and maybe make the fixed 15s period an unexported configuration option, so we can set it to 0 in tests)?
There was a problem hiding this comment.
Updated channel close logic to wait for minimum 15 seconds irrespective of custom grace time.
🤖 I have created a release *beep* *boop* --- ## [1.96.0](spanner/v1.95.1...spanner/v1.96.0) (2026-10-07) ### Features * **all:** Update supported go versions ([#20520](#20520)) ([90413d3](90413d3)) * **spanner:** Add experimental vtprotobuf decoding with ExperimentalBorrowRows under the spanner_vtproto build tag ([#20642](#20642)) ([fdbd4f4](fdbd4f4)) ### Bug Fixes * **spanner:** Keep DCP transaction on its channel while it drains ([#20618](#20618)) ([93dfc20](93dfc20)) * **spanner:** Prioritize leader replica for read-write transactions in location-aware routing ([#20620](#20620)) ([a524d66](a524d66)) * **spanner:** Record streaming built-in metrics per result stream ([#20596](#20596)) ([be75f7e](be75f7e)) * **spanner:** Retry and resume BatchWrite streams on UNAVAILABLE ([#20623](#20623)) ([a302277](a302277)) ### Performance Improvements * **spanner:** Skip builtin metrics tracer allocation when metrics are disabled ([#20582](#20582)) ([9971a0c](9971a0c)) * **spanner:** Skip reading response headers when no latency sink is enabled ([#20583](#20583)) ([3dcc2c3](3dcc2c3)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
With the dynamic channel pool, a session handle re-resolved its bound channel by id among active entries on every RPC, so a read-write transaction moved to another channel as soon as scale-down marked its channel draining. Keep a pointer to the bound entry instead and use it while it is active or draining; re-pick only once it is closed.