Skip to content

feat(bigtable): add ChainInterceptors and RetryingVRpc for vRPC pipeline - #20185

Merged
sushanb merged 5 commits into
googleapis:mainfrom
sushanb:feat/bigtable-vrpc-retrying
Jul 22, 2026
Merged

feat(bigtable): add ChainInterceptors and RetryingVRpc for vRPC pipeline#20185
sushanb merged 5 commits into
googleapis:mainfrom
sushanb:feat/bigtable-vrpc-retrying

Conversation

@sushanb

@sushanb sushanb commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

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 multiple Interceptors around a base Handler. First interceptor in the list is outermost, so it sees setup before and teardown after the rest.
  • RetryingVRpc (internal/transport/retrying.go) — an Interceptor that retries failed virtual RPCs with exponential backoff. Default classification is Java-parity:
    • StateUncommitted → always retry (server never saw the frame).
    • StateTransportFailure → retry only when Idempotent is true (server may have applied).
    • StateServerResult → retry only if the server attached errdetails.RetryInfo; a bare server-explicit error (even Unavailable / Aborted / DeadlineExceeded) does NOT retry without explicit server go-ahead.
    • Callers with bespoke 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.

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/completes
    • TestRetryingVRpc_NonRetryableErrorInvalidArgument aborts after 1 attempt
    • TestRetryingVRpc_HonorServerRetryDelay — server RetryInfo overrides client backoff
    • TestRetryingVRpc_MaxAttemptsExceeded — stops at MaxAttempts
    • TestRetryingVRpc_NonGrpcError — raw Go errors are non-retryable
    • TestRetryingVRpc_UncommittedAlwaysRetriesStateUncommitted retries even when Idempotent=false
    • TestRetryingVRpc_TransportFailureIdempotentStateTransportFailure retries with Idempotent=true
    • TestRetryingVRpc_TransportFailureNonIdempotentNoRetry — no retry when Idempotent=false
    • TestRetryingVRpc_ServerResultNotRetriedByDefault — bare ServerResult never retries (5 codes)
    • TestRetryingVRpc_ServerDeadlineExceededNoRetryByDefault — server DEADLINE_EXCEEDED alone is not retried
    • TestRetryingVRpc_ServerDeadlineExceededRetriesWithRetryInfoDEADLINE_EXCEEDED + RetryInfo is retried
  • goimports -d clean
  • golint clean

…/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).
@sushanb
sushanb requested review from a team as code owners July 21, 2026 20:36
@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Jul 21, 2026
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@sushanb

sushanb commented Jul 21, 2026

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 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.

Comment thread bigtable/internal/transport/retrying.go Outdated
Comment thread bigtable/internal/transport/retrying.go Outdated
Comment thread bigtable/internal/transport/chain.go
sushanb added 3 commits July 21, 2026 20:47
- 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.
sushanb added a commit to sushanb/google-cloud-go that referenced this pull request Jul 22, 2026
…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.
Comment thread bigtable/internal/transport/retrying.go Outdated
Comment thread bigtable/internal/transport/retrying.go Outdated
Comment thread bigtable/internal/transport/retrying.go Outdated
Comment thread bigtable/internal/transport/retrying.go Outdated
Comment thread bigtable/internal/transport/retrying.go Outdated
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.
@sushanb

sushanb commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Addressed all five review comments in 621817a:

  • Java references stripped from retrying.go and retrying_test.go.
  • InitialBackoff / MaxBackoff / BackoffMultiplier removed. Backoff is now server-driven: honor RetryInfo.retryDelay if present, otherwise retry immediately. No client-side exponential backoff.
  • MaxAttempts cap applies only to non-server-directed retries (Uncommitted or TransportFailure without RetryInfo). Server-directed retries are uncapped, bounded only by ctx deadline.
  • ShouldRetry field removed. Zero production callers; server RetryInfo short-circuit already ensured "server always wins". Field was dead weight.
  • serverPermitsRetry 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".

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.

15 tests pass under -race; full transport suite green.

Follow-up (sessionz-side, not this PR): SESSION_SPEC.md line 81's "3-attempt cap" row currently says the cap "overrides any amount of server RetryInfo". That text is now stale. Spec file lives on feat/bigtable-sessionz-debug, not upstream/main, so the edit will land alongside the sessionz-side port of this PR.

@sushanb
sushanb merged commit c7a832a into googleapis:main Jul 22, 2026
19 checks passed
hongalex pushed a commit that referenced this pull request Jul 23, 2026
🤖 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the Bigtable API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants