Repository navigation
Conversation
Use a copy-on-write test metric recorder because synchronized-list iteration raced with asynchronous exports.
With dynamic scaling enabled, **maybeDynamicUpscale()** ran synchronously on the caller thread during channel selection, before the stream was counted. It scaled on the pool-wide average only, one channel per trigger, with no cooldown — so a burst could add channels one RPC at a time while the caller waited on channel construction, and a single hot channel never triggered growth if the average stayed low. **Change** - Signal after accounting. **activeStreamsCountIncr()** evaluates the trigger once the stream is reserved: the selected channel above maxRpcPerChannel, or the pool average above it, signals scale-up. Channel selection itself no longer scales. - Single background worker. A one-slot signal (AtomicBoolean pending/running) coalesces bursts; the worker runs on the shared background executor, loops while signals arrive, and catches **Throwable** so one failure can't stop future scale-ups. The scale-down timer body gets the same guard. - Bounded, load-sized growth. Desired size = **ceil(totalActiveStreams / midpoint(minRpc, maxRpc))**, added channels capped at **maxScaleUpPercent** of the active pool (floor 2) and by maxSize. A scaleUpCooldown (default 10s) is claimed before channel construction so signals arriving mid-build are throttled. READY channels awaiting shutdown are reused before new ones are built. - Construction outside the lock. Delegate channels are built without holding the pool monitor; publication rechecks shutdown and headroom under the lock and closes surplus channels. - Inactive channels excluded from round-robin, fallback, extrema (getMin/MaxActiveStreams) and picker scans; the legacy watermark scale-up no longer computes getMaxActiveStreams() on the hot path when dynamic scaling is enabled. - Shutdown ordering: shuttingDown is set under the pool lock as the first step of shutdown()/shutdownNow(), so a channel cannot be published after shutdown begins. removedChannelRefs is now a concurrent set, safe for the worker and the unsynchronized readers (isShutdown, isTerminated, awaitTermination, stats logging).
With dynamic scaling enabled, scale-down removed the longest-connected channels abruptly: the channel disappeared from the pool while callers still held it, affinity mappings were dropped without going through normal unbind accounting, and the removed channel was only shut down when a later scale-down tick happened to observe zero active streams. RPCs and transaction affinities pinned to a removed channel could land on a closing delegate, and the picker could crash (IndexOutOfBoundsException / IllegalStateException) when the pool shrank mid-scan. **Change** - **Scale-down drains instead of dropping.** `removeChannels()` picks the least-loaded channels first (fewest bound affinities, then age, break ties), deactivates each one *before* publishing the shorter active list, and moves it to the draining set. In-flight RPCs finish on the draining delegate; new picks skip inactive channels. Affinity-key release and drain-timer arming happen after the pool lock is released. - **Draining is one-way** (matching the Go reference): a draining channel is never returned to the active pool; scale-up always builds a fresh channel. Removes the legacy `pickChannelForReuse` path. - **Idle-grace shutdown.** A draining channel shuts down only after drainIdleGrace` (new pool option, default 1 min) of inactivity with zero active streams. The last stream completion arms the timer directly instead of waiting for the next scale-down tick. Drain timers carry their own identity, so a stale timer that fires late cannot cancel or act on behalf of a newer one; timer bookkeeping is guarded by the pool monitor while delegate `shutdown()` always runs outside it. If the scheduler rejects a drain task, the channel is closed immediately rather than leaked; all pending timers are cancelled on `shutdown()`/`shutdownNow()`. - **Affinity consistency.** Keys bound to removed channels are released through the normal unbind path, keeping per-channel and aggregate affinity counts correct. Key lookup tolerates concurrent rebinds without leaking or regressing last-used timestamps (`merge` with `Long::max`), a mapping to an inactive channel is atomically replaced (looping until an active mapping or a fresh bind), and `bind()` returns the channel actually bound so callers never hand out a stale draining reference. Caller-owned affinity refs stay sticky on a draining channel until its delegate shuts down, preserving transaction affinity across scale-down. - **Picker hardening.** Pool scans use the CopyOnWriteArrayList snapshot iterator (bounds-safe under concurrent shrink by construction), load comparisons go through `getPickerLoad()`, the overload watermark uses the configured `maxConcurrentStreamsLowWatermark` instead of the DEFAULT_MAX_STREAM constant, and an exhausted pool fails with a retryable `UNAVAILABLE` status instead of IllegalStateException. Scale-up sizes itself from active channels only, with division guards for transiently negative load sums. - **Readiness accounting** deltas are computed under the channel lock but applied outside it, removing the lock-order inversion between the channel and pool monitors.
With dynamic scaling enabled, a channel returning retryable errors (UNAVAILABLE / RESOURCE_EXHAUSTED) kept its low active-stream count and so kept attracting picks: the pool had no signal that a channel was unhealthy until its streams actually piled up, and scale-up never triggered for a pool that was failing rather than busy. **Change** - **Error penalty on retryable failures.** A call completing with UNAVAILABLE or RESOURCE_EXHAUSTED adds `errorPenaltyStep` (new pool option, default 5) of synthetic load to that channel, capped at `maxRpcPerChannel`. The penalty **decays linearly to zero** over `errorPenaltyDuration` (default 5s), so a recovered channel re-enters picking gradually instead of at a cliff; a new retryable error restores the accumulated penalty and resets the window. Picker reads are lock-free and use a single clock snapshot per pick. - **Scale-up integration.** Penalty load counts toward the dynamic scale-up trigger with a strict capacity comparison, and applying a penalty signals the scale-up check eagerly — so a pool that is failing-but-idle still grows (bounded: one channel at a time). - **Single net-delta aggregate accounting.** Every update to the shared penalty aggregate is computed under the channel lock and applied as one atomic delta, so lock-free readers never observe an intermediate state; a channel leaving the pool clears its contribution. The aggregate deliberately holds the undecayed value until expiry as a conservative scale-up bound. - **Configuration.** `setErrorPenaltyStep(0)` disables penalties (negative rejected); `errorPenaltyDuration` must be strictly positive.
With dynamic scaling enabled, a freshly built channel was published to the pool the moment its delegate was constructed. Its first real RPCs then paid for connection establishment, TLS, and any service-side warm-up, so every scale-up event injected a latency spike into live traffic right when the pool was already under load. **Change** - **Optional channel primer.** New `GcpChannelPrimer` hook (`setChannelPrimer`) issues a cheap end-to-end RPC on each newly built scale-up channel before it is published; for Cloud Spanner this can be a `SELECT 1`. Channels in a scale-up batch are primed concurrently and each one is published as soon as its own prime succeeds, so one slow channel never holds back the rest. A `null` primer (the default) keeps the existing publish-on-build path. Only dynamic scale-up channels are primed; the initial pool is not. - **Bounded attempts.** Each attempt is bounded by `channelPrimeTimeout` (default 10s) and retried up to `channelPrimeMaxAttempts` (default 3) with exponential backoff (100ms doubling, capped at 5s). The timeout covers the whole attempt, including the synchronous `prime()` call. A primer still blocked inside `prime()` at the timeout fails the channel without a retry, since nothing can stop that call; primer invocations run on a dedicated executor so a blocked primer cannot starve the shared scheduler that drives scale-up, draining, and timeouts. Exhausted or timed-out channels are closed and counted in the new `scale_up_prime_failures` metric. - **Scale-up accounting.** Channels still priming count toward both the pool size cap and the desired size, so a second scale-up during a slow prime batch cannot rebuild the same shortfall or construct delegates past `maxSize`. A prime abandoned because its call never returned keeps its slot until the call does, so a stuck primer can pin at most `maxSize` slots rather than one thread per scale-up event. - **Lifecycle.** Unpublished priming channels are never visible to pickers; shutdown cancels every pending prime and closes its delegate. Per-RPC pick and stream-accounting paths are unchanged, with no new hot-path allocations. - **Configuration.** `setChannelPrimeTimeout` rejects negative or non-nanosecond-representable durations (zero uses the default); `setChannelPrimeMaxAttempts` rejects negatives (zero uses the default).
…ECT 1 (googleapis#14254) The grpc-gcp dynamic channel pool (DCP) now supports a GcpChannelPrimer that runs before a scaled-up channel is published to the pool, but java-spanner never registered one, so channels added under load were published cold: the first requests on them paid for the TCP handshake, TLS session, and server-side connection setup at the moment the pool was already saturated. The Go Spanner client primes scaled-up channels by executing SELECT 1 with the multiplexed session through the new channel. This change brings java-spanner to parity.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic channel priming and enhanced load-balancing mechanisms to the gRPC GCP Java client, with specific integration for Google Cloud Spanner. Key additions include a GcpChannelPrimeController and GcpChannelPrimer interface to warm up newly scaled-up channels using multiplexed sessions before publishing them to the pool. It also implements decaying error penalties for channels experiencing transient failures, a more robust channel draining process with idle grace periods, and refined channel selection strategies (such as Power-of-Two with replacement). Additionally, the changes address potential active stream count leaks and thread-safety issues during pool shutdown. Regarding the review feedback, a thread-safety concern was raised in GcpChannelPrimeController where several shared state fields (future, timeoutTask, and retryTask) are read in the cancel method without holding the pool lock, which could lead to consistency issues.
Comment on lines
+90
to
+92
| @Nullable private ListenableFuture<Void> future; | ||
| @Nullable private ScheduledFuture<?> timeoutTask; | ||
| @Nullable private ScheduledFuture<?> retryTask; |
Contributor
There was a problem hiding this comment.
The fields future, timeoutTask, and retryTask are read in the cancel method without holding the pool lock. To ensure thread safety and prevent potential resource leaks, prefer guarding these multiple shared state variables with a lock rather than relying on multiple volatile variables, as volatile variables can be difficult to reason about and order correctly to ensure consistency.
References
- In concurrent programming, prefer guarding multiple shared state variables with a lock rather than relying on multiple volatile variables, as volatile variables can be difficult to reason about and order correctly to ensure consistency.
lqiu96
force-pushed
the
backport-1.90.x-spanner-grpc-gcp
branch
from
October 30, 2026 01:12
a6e1531 to
739ebc0
Compare
lqiu96
marked this pull request as ready for review
October 30, 2026 15:53
lqiu96
changed the title
lqiu96
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
blakeli0
approved these changes
Oct 30, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.