feat(bigtable): add ChainInterceptors and RetryingVRpc for vRPC pipeline - #20185
Conversation
…/interceptor pipeline Add two building blocks on top of the vRPC primitives merged in googleapis#20116: * ChainInterceptors composes multiple Interceptors around a base Handler (first interceptor is outermost, so it sees setup before and teardown after the rest). * RetryingVRpc is an Interceptor that retries failed virtual RPCs with exponential backoff. The default classification is Java-parity: Uncommitted attempts always retry, TransportFailure retries only when the op is idempotent, and ServerResult retries only if the server attached errdetails.RetryInfo. Callers with unusual policies can set ShouldRetry to override the default entirely. Per-attempt tracing goes through VRpcTracer; the previous attempt's error is tagged onto the next attempt's context via WithPrevAttemptErr for downstream metrics/logging. Tests cover chain ordering, success-on-retry, non-retryable and non-gRPC errors, honoring server RetryDelay (including for DEADLINE_EXCEEDED carrying RetryInfo), max-attempts exhaustion, and the state-based classification matrix (Uncommitted always retries; TransportFailure gated by Idempotent; bare ServerResult never retries by default).
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces interceptor chaining and retry mechanisms for virtual RPCs in the Bigtable client, along with comprehensive unit tests. The feedback recommends optimizing ChainInterceptors for cases with zero or one interceptors, simplifying the retry loop by removing redundant atomic operations on local variables, and replacing time.After with time.NewTimer to prevent potential memory leaks when a context is canceled.
- chain.go: fast paths for len(interceptors) == 0 (no-op wrapper) and == 1 (return interceptor as-is), matching gRPC's own ChainUnaryInterceptor convention. Avoids the nested-closure alloc on every RPC in degenerate cases. - retrying.go: local attempt counter dropped from atomic.AddInt32 to plain int32 (goroutine-local, no synchronization needed). Kept the atomic.LoadInt32 on opts.MaxAttempts — that field's doc says "Atomic race cap" and external code (config-manager fan-out) may swap it while the loop runs. - retrying.go: swapped time.After for time.NewTimer + explicit Stop() on the ctx-cancel exit path so the timer is released immediately instead of leaking for the full backoff duration.
… lastErr, RetryInfo authority Three behavioral-review findings against SESSION_SPEC #9 addressed: 1. Deadline-fit check missing on RetryInfo delay. When the server-directed delay would exhaust the caller's remaining deadline, the loop now returns lastErr instead of sleeping past the deadline. Spec quote: "RetryInfo.retryDelay is IGNORED (not clipped) if it would exhaust the caller's remaining deadline" (RetryingVRpc.java:290-298 parity). Applied uniformly to any delay — server RetryInfo or client backoff. 2. lastErr dropped on ctx-cancel. Two sites returned raw ctx.Err() instead of the last observed typed error. Callers lost the gRPC code + AttemptState tag they needed to reason about what failed. Both sites now return lastErr — matches Java behavior. 3. ShouldRetry blind to serverPermitsRetry. A caller-supplied ShouldRetry callback could veto a server-directed retry, defeating the whole point of the RetryInfo detail. Server RetryInfo is server-only authority per SESSION_SPEC #9 and now short-circuits above both the ShouldRetry and default gates. Also: dropped the "atomic-swappable MaxAttempts" indirection. No matching field exists in ClientConfiguration proto — polling has MaxRpcRetryCount for GetClientConfiguration itself, not data vRPCs. The atomic.LoadInt32 was aspirational plumbing; capture MaxAttempts into a closure-local at RetryingVRpc(...) return instead. Three new tests pin each fix: - TestRetryingVRpc_CtxCancelPreservesLastErr — sentinel survives outer cancel. - TestRetryingVRpc_DeadlineFitSkipsRetry — 20ms deadline + 5s server delay = 1 attempt, no sleep. - TestRetryingVRpc_ServerRetryInfoOverridesShouldRetry — ShouldRetry=always-false + server RetryInfo still retries.
…apis#20185 Ports the three retry-oracle fixes just landed on feat/bigtable-vrpc- retrying (c90d5b9, PR googleapis#20185) into the sessionz branch so both trees stay on the same behavior. Reviewer-verified against SESSION_SPEC #9 + SESSION_COMPONENT_SPEC.md. 1. Deadline-fit check: skip retry when the delay would exhaust the caller's remaining ctx deadline (server RetryInfo OR client backoff). Returns lastErr instead of sleeping past the deadline. Java parity RetryingVRpc.java:290-298. 2. lastErr preserved on ctx-cancel: both the post-attempt and backoff-window cancel branches now return the typed lastErr instead of raw ctx.Err(), so callers keep the gRPC code + AttemptState tag. 3. Server RetryInfo overrides ShouldRetry: the caller-supplied ShouldRetry callback and the default gate are both wrapped in `if !serverPermitsRetry {...}`. Per SESSION_SPEC #9 server-only inputs, RetryInfo is server-side authority; client policy cannot veto it. Also drops the unused sync/atomic ceremony on the retry loop's local attempt counter and MaxAttempts read. `attempt` is closure-local single goroutine; MaxAttempts has no server-config field to swap, so the atomic pattern was aspirational plumbing. Captured to closure via `maxAttempts := opts.MaxAttempts` at RetryingVRpc(...) return. Three new tests in vrpc_test.go pin each fix: - TestRetryingVRpc_CtxCancelPreservesLastErr — sentinel survives outer cancel. - TestRetryingVRpc_DeadlineFitSkipsRetry — 20ms deadline + 5s server delay → 1 attempt. - TestRetryingVRpc_ServerRetryInfoOverridesShouldRetry — ShouldRetry=always-false + server RetryInfo still retries. Full transport suite green under -race -short.
Addresses five inline comments on PR googleapis#20185: 1. Strip Java references from doc comments in retrying.go and retrying_test.go (nit). 2. Drop InitialBackoff / MaxBackoff / BackoffMultiplier fields from RetryingOptions. Backoff is server-driven: honor RetryInfo delay if the server sent one, otherwise retry immediately. No client-side exponential backoff. Matches Java RetryingVRpc — retryDelay==0 → immediate onStateChange(new Idle()). 3. MaxAttempts cap applies ONLY to non-server-directed retries (Uncommitted or TransportFailure with no RetryInfo). Server- directed retries are uncapped, bounded only by ctx deadline — Java parity. 4. Drop the ShouldRetry field. No production caller sets it; the field was defensive future-plumbing that never materialized. Server RetryInfo already short-circuits the state-based gate; the ShouldRetry override was redundant. 5. serverPermitsRetry now gated on RetryDelay.IsValid() — an empty or malformed RetryInfo detail is not enough to bypass the state-based default. Only a well-formed retryDelay counts as the server saying "retry". Matches Java's hasRetryDelay() gate. Two new tests pin the redesign: - TestRetryingVRpc_ServerRetryInfoUncapsMaxAttempts — MaxAttempts=3 + server RetryInfo → succeeds at attempt 6. - TestRetryingVRpc_NoClientBackoffOnNonServerRetry — 3 back-to-back TransportFailure retries elapse < 50ms (no client-side backoff). Test coverage now 15 tests (was 14). All pass under -race. Note: SESSION_SPEC.md line 81 (Client-only inputs table, "3-attempt cap" row) currently says "hard cap ... overrides any amount of server RetryInfo" — that text is now stale under the new (Java-parity) code. The spec file lives on feat/bigtable-sessionz-debug, not on upstream/ main, so the spec edit will land alongside the eventual sessionz-port of this PR.
|
Addressed all five review comments in 621817a:
Two new tests pin the redesign:
15 tests pass under Follow-up (sessionz-side, not this PR): |
🤖 I have created a release *beep* *boop* --- ## [1.51.0](bigtable/v1.50.0...bigtable/v1.51.0) (2026-07-23) ### Features * **bigtable:** Add ChainInterceptors and RetryingVRpc for vRPC pipeline ([#20185](#20185)) ([c7a832a](c7a832a)) * **bigtable:** Add ClientConfigurationManager ([#19986](#19986)) ([3a8f927](3a8f927)) * **bigtable:** Add debug tag counter (recordDebugTag / assertDebugTag) ([#20114](#20114)) ([3c97590](3c97590)) * **bigtable:** Add lazyPool helper for on-demand session pool opening ([#20182](#20182)) ([f6ae3fb](f6ae3fb)) * **bigtable:** Add PeakEwma continuous time-decay latency tracker ([#20187](#20187)) ([9d124ef](9d124ef)) * **bigtable:** Add PoolSizer for server-driven session pool capacity ([#20189](#20189)) ([57ebbeb](57ebbeb)) * **bigtable:** Add session package with SessionClient + SessionTableAPI interfaces ([#20180](#20180)) ([4b82fd2](4b82fd2)) * **bigtable:** Add Session primitives (AttemptOutcome, vRPC ctx, msgtype) ([#20116](#20116)) ([e1011e2](e1011e2)) * **bigtable:** Add Session state enum ([#19981](#19981)) ([0748972](0748972)) * **bigtable:** Add SessionThrottler / AdaptiveSessionThrottler for OpenSession pacing ([#20184](#20184)) ([02e3c6d](02e3c6d)) * **bigtable:** Add SessionThrottler / AdaptiveSessionThrottler for OpenSession pacing ([#20184](#20184)) ([29be83e](29be83e)) * **bigtable:** Add sessionTracer for per-Session lifecycle + vRPC metrics ([#20190](#20190)) ([a466345](a466345)) * **bigtable:** Enable new auth library and JWT for instance admin client ([#20013](#20013)) ([21c4a44](21c4a44)) * **bigtable:** Modularize channel priming behind a ChannelPrimer interface ([#20027](#20027)) ([5214ab7](5214ab7)) * **bigtable:** Modularize Direct Access compatibility check ([#19987](#19987)) ([a25e93d](a25e93d)) * **o11y:** Regenerate clients for LRO tracing ([#20107](#20107)) ([779074e](779074e)) ### Bug Fixes * **bigtable:** Default cluster/zone in toOtelMetricAttrs to avoid Monitoring reject ([#20178](#20178)) ([14493f4](14493f4)) * **bigtable:** Eliminate stats-handler MD race in internal/metrics tracer ([#20158](#20158)) ([c387066](c387066)) --- 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>
Summary
Builds on the vRPC primitives merged in #20116 by adding the two building blocks that consumers need to actually run interceptor pipelines with retries:
ChainInterceptors(internal/transport/chain.go) — composes multipleInterceptors around a baseHandler. First interceptor in the list is outermost, so it sees setup before and teardown after the rest.RetryingVRpc(internal/transport/retrying.go) — anInterceptorthat retries failed virtual RPCs with exponential backoff. Default classification is Java-parity:StateUncommitted→ always retry (server never saw the frame).StateTransportFailure→ retry only whenIdempotentis true (server may have applied).StateServerResult→ retry only if the server attachederrdetails.RetryInfo; a bare server-explicit error (evenUnavailable/Aborted/DeadlineExceeded) does NOT retry without explicit server go-ahead.ShouldRetryto override the default entirely.Per-attempt tracing goes through
VRpcTracer; the previous attempt's error is tagged onto the next attempt's context viaWithPrevAttemptErrfor downstream metrics/logging.Test plan
go build ./internal/transport/...go vet ./internal/transport/...go test ./internal/transport/... -run 'Retrying|Chained|Interceptor' -count=1 -race -timeout=90s— 12 new tests pass:TestChainedInterceptors_Success— interceptor call ordering (outer→inner→base→inner→outer)TestRetryingVRpc_SuccessOnRetry— recovers on attempt 3, tracer sees all 3 starts/completesTestRetryingVRpc_NonRetryableError—InvalidArgumentaborts after 1 attemptTestRetryingVRpc_HonorServerRetryDelay— serverRetryInfooverrides client backoffTestRetryingVRpc_MaxAttemptsExceeded— stops atMaxAttemptsTestRetryingVRpc_NonGrpcError— raw Go errors are non-retryableTestRetryingVRpc_UncommittedAlwaysRetries—StateUncommittedretries even whenIdempotent=falseTestRetryingVRpc_TransportFailureIdempotent—StateTransportFailureretries withIdempotent=trueTestRetryingVRpc_TransportFailureNonIdempotentNoRetry— no retry whenIdempotent=falseTestRetryingVRpc_ServerResultNotRetriedByDefault— bareServerResultnever retries (5 codes)TestRetryingVRpc_ServerDeadlineExceededNoRetryByDefault— serverDEADLINE_EXCEEDEDalone is not retriedTestRetryingVRpc_ServerDeadlineExceededRetriesWithRetryInfo—DEADLINE_EXCEEDED+RetryInfois retriedgoimports -dcleangolintclean