feat(grpc-gcp): prime scaled channels before publish - #14232
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a channel priming mechanism (GcpChannelPrimer) to warm up newly built delegate channels during dynamic scale-up before they are published to the channel pool. It adds options to configure the primer, timeout, and maximum retry attempts, along with a dedicated cached thread pool to execute priming tasks asynchronously without blocking the shared background scheduler. Additionally, it updates the dynamic scale-up logic to account for in-flight priming channels to prevent over-provisioning, integrates a new metric (scale_up_prime_failures) to track priming failures, and includes comprehensive unit tests validating the priming lifecycle, timeouts, retries, and shutdown behavior. There are no review comments, so I have no feedback to provide.
| /** A channel management factory that implements grpc.Channel APIs. */ | ||
| public class GcpManagedChannel extends ManagedChannel { | ||
|
|
||
| private static final class PendingPrime { |
There was a problem hiding this comment.
Can we move both this inner class and most (all?) of the logic that is being added here to a separate file instead of adding even more to GcpManagedChannel? Or is it strictly required that we add it here?
| @VisibleForTesting | ||
| int inFlightPrimeCountForTest() { | ||
| return inFlightPrimeCount.get(); | ||
| } |
There was a problem hiding this comment.
nit: This can probably just return pendingPrimes.size(). But can you also check whether we really need both this and the other methods here that are only added for tests?
| } | ||
|
|
||
| private static long primeBackoffMillis(int attempt) { | ||
| return Math.min(100L << Math.min(attempt, 12), 5000L); |
There was a problem hiding this comment.
Can we add some random jitter here to spread the retries a bit when we are creating multiple channels at the same time?
| /** | ||
| * Issues a cheap end-to-end RPC on {@code channel} so its connection is warm before real traffic. | ||
| * Scale-up batches invoke this method concurrently across channels and publish each channel as | ||
| * soon as its own future succeeds. For example, a Cloud Spanner implementation can execute {@code | ||
| * SELECT 1}. Return a failed future to reject the attempt; the pool retries up to the configured | ||
| * attempt count and closes the channel when they are exhausted. | ||
| * | ||
| * <p>Return promptly and do the work inside the future. The configured prime timeout bounds each | ||
| * attempt as a whole: a future still pending at the timeout is cancelled and the attempt retried, | ||
| * while a call that is still blocked inside this method at the timeout fails the channel with no | ||
| * retry, because nothing can stop it. A retry may overlap work from a cancelled future if that | ||
| * work does not promptly honor cancellation, so implementations must tolerate concurrent calls | ||
| * for one channel. | ||
| */ |
There was a problem hiding this comment.
Can we extend this comment a bit further to explain what implementers need to take into account when implementing this method. Something like this (this came up when I started worrying and asking questions around the fact that Spanner's ExecuteStreamingSql has a default 1hr timeout):
/**
* Issues a cheap, non-mutating end-to-end RPC on {@code channel} so that its underlying transport,
* TLS session, and server-side connections are fully established before the channel serves live traffic.
*
* <p>Scale-up batches invoke this method concurrently across channels, and each channel is published
* to the pool as soon as its own future completes successfully. For example, a Cloud Spanner
* implementation might execute {@code SELECT 1}. Returning a failed future rejects the attempt;
* the pool retries up to the configured max attempts before discarding and closing the channel.
*
* <h3>Implementation Requirements & Best Practices:</h3>
* <ul>
* <li><b>Return promptly — Do NOT block:</b> This method must return a {@link ListenableFuture}
* immediately without performing synchronous or blocking work (e.g., avoid {@code blockingStub},
* {@code ResultSet.next()}, {@code Future.get()}, or thread synchronization). Blocking inside
* this method starves the primer executor and, if timed out, pins pool scale-up capacity.</li>
*
* <li><b>Set an explicit, short RPC deadline:</b> The underlying gRPC call <em>MUST</em> be
* configured with an explicit, short deadline (e.g., {@code stub.withDeadlineAfter(5, SECONDS)}).
* Many Google Cloud APIs (such as Cloud Spanner's streaming queries) have default deadlines of
* up to 1 hour. Never rely on default RPC deadlines; a slow or unresponsive backend could leave
* calls lingering indefinitely on the server.</li>
*
* <li><b>Propagate cancellation to the gRPC call:</b> When the pool's configured prime timeout
* expires, it invokes {@code future.cancel(true)} on the returned future. Implementers must
* ensure that cancelling this future actively cancels the underlying gRPC {@link io.grpc.ClientCall}
* or {@link io.grpc.Context.CancellableContext} so transport streams and server-side resources
* are terminated immediately.</li>
*
* <li><b>Tolerate concurrent calls:</b> If a cancelled RPC does not terminate immediately on the
* wire, a subsequent retry attempt may be issued while the previous attempt is still winding down.
* Implementations must be safe for multiple concurrent invocations on the same channel.</li>
*
* <li><b>Use trivial, read-only operations:</b> The operation should be strictly read-only,
* side-effect free, and require minimal server compute (e.g., {@code SELECT 1} or a lightweight ping).</li>
* </ul>
*
* @param channel the newly created, unpublished delegate channel to prime
* @return a {@link ListenableFuture} that completes when the priming RPC succeeds, or fails to trigger a retry
*/
ListenableFuture<Void> prime(ManagedChannel channel);| if (blockedInPrimer || shuttingDown || attempt + 1 >= channelPrimeMaxAttempts) { | ||
| finishPendingPrime(pendingPrime); | ||
| if (blockedInPrimer && !shuttingDown) { | ||
| abandonedPrimes.add(pendingPrime); |
There was a problem hiding this comment.
Should we log a warning for this? It happens completely in the background and is invisible to the application (I know that we have a metric for it, but not everyone is collecting that, and it would be a short blip in the overall metrics if it only happens a few times)
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
GcpChannelPrimerhook (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 aSELECT 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. Anullprimer (the default) keeps the existing publish-on-build path. Only dynamic scale-up channels are primed; the initial pool is not.channelPrimeTimeout(default 10s) and retried up tochannelPrimeMaxAttempts(default 3) with exponential backoff (100ms doubling, capped at 5s). The timeout covers the whole attempt, including the synchronousprime()call. A primer still blocked insideprime()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 newscale_up_prime_failuresmetric.maxSize. A prime abandoned because its call never returned keeps its slot until the call does, so a stuck primer can pin at mostmaxSizeslots rather than one thread per scale-up event.setChannelPrimeTimeoutrejects negative or non-nanosecond-representable durations (zero uses the default);setChannelPrimeMaxAttemptsrejects negatives (zero uses the default).