feat(bigtable): add ClientConfigurationManager - #19986
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a ClientConfigurationManager to dynamically poll and manage client configurations from the Bigtable control plane. The review feedback identifies several critical issues: the background polling loop is bound to a potentially short-lived context in Start which could prematurely terminate polling; a race condition in addListener can allow stale configurations to overwrite newer ones; an integer overflow in the exponential backoff calculation (1<<i) can cause a runtime panic in rand.Intn; and the fallback mechanism fails to reset the validity window (m.validUntil), resulting in redundant fallback notifications during prolonged outages.
ClientConfigurationManager polls GetClientConfiguration on a fixed interval, parses the response into a typed clientConfig, and fans changes out to registered listeners. Supports RPC retry with capped randomized exponential backoff, a validity-window fallback to defaults on prolonged outage, and Close() shutdown that waits for in-flight polls so listener callbacks cannot fire against state that is about to be torn down. Listener delivery is monotonic-by-sequence (CAS-guarded against the register-then-fire race with concurrent polls). Two cheap diff layers suppress redundant work: a whole-config == check in poll() skips the fan-out entirely when the polled config is unchanged, and the typed AddSessionPoolListener / AddSessionLoadListener wrappers each hold the last value they delivered and short-circuit when their own slice is unchanged — mirroring Java's ListenerEntry.maybeNotify pattern. Wired into Client behind a new ClientConfig.EnableSession flag (off by default). When set, NewClientWithConfig constructs the manager using the data-plane BigtableClient + instance-scoped request metadata and Start()s it; Close() stops the manager first so it cannot fire listeners against a tearing-down pool. The session-pool ChannelPrimer / DirectAccessChecker / pool-factory wiring that consumes this manager is intentionally deferred to a follow-up PR for review scope.
fed66e8 to
aedceca
Compare
|
Scope reduction — force-pushed to Narrowed this PR down to just the ClientConfigurationManager + the Previous tip ( |
…backoff Address review on googleapis#19986: - Per-field doc comments on pollingConfig, sessionConfig, channelPoolConfig, sessionPoolConfig, and loadBalancingOptions so the config surface is self-describing rather than implied by the proto field names. - Expand the configListener type doc to specify what seq carries (the manager's monotonic configSeq at fire time) and how listeners can rely on monotonicListener's guard against reordered deliveries. - Move the defaultClientConfig literal into its own default_client_config.go so it reads as a tunable defaults table separate from the polling machinery. - Hoist the 5s per-RPC poll timeout into a named pollDeadline const reused by both Start()'s eager initial poll and pollingLoop()'s periodic polls, instead of two open-coded 5*time.Second literals. - Collapse the retry-backoff cap to a one-liner using built-in min: min(1<<min(i, 30), maxBackoffSeconds). Preserves the overflow guard (shift width capped at 30) and the absolute backoff cap, and drops the explicit <=0 fallback branch that's no longer reachable.
…onManager Replace the trio of done chan / closeOnce / closed atomic.Bool with a stored ctx + cancel pair plus the same closed gate driven by CompareAndSwap. - ctx, cancel are derived from Start()'s parent ctx and stored on the manager. Close() calls cancel() to wake pollingLoop and abort any in-flight poll RPC — replacing the done channel + watcher goroutine inside pollingLoop that previously translated <-done into cancel(). - CompareAndSwap(false, true) on closed makes Close idempotent (replacing sync.Once, which was only there to keep close(done) from panicking on a second call) and continues to arm the read-side gate poll() consults before firing listeners. - pollsWG.Wait() in Close still guarantees in-flight polls return before Close does. Side benefit: the eager initial poll spawned by Start() is now scoped to m.ctx instead of the caller's ctx, so Close()'s cancel reaches it too — previously a Close() called within the first pollDeadline window would block in pollsWG.Wait for up to 5s waiting on the initial RPC. cancel is initialized to a no-op in New, so a Close-before-Start call remains safe.
Pull the GetClientConfiguration RPC + randomized-exponential-backoff retry loop out of poll() into a dedicated fetchClientConfiguration helper. poll() now reads as: fetch, handle ctx cancellation, handle RPC error (validity-window fallback), apply successful response. The helper returns ctx.Err() immediately when ctx is cancelled mid-backoff, and poll() checks ctx.Err() up front before any fallback-to-default work. Side benefit: this closes the small edge case where ctx cancellation during the *last* RPC attempt would fall through the loop into the fallback branch and fire listeners during shutdown — the explicit ctx.Err() check handles that uniformly now.
…icListener addListener now holds m.mu across the registration-time fire instead of wrapping every listener in a monotonicListener / atomic-CAS guard. Holding the write lock serializes the initial (cfg, seq) delivery with poll()'s fan-out: poll() cannot snapshot the listener map (and therefore cannot fire this listener with a newer seq) until addListener releases the lock, so listeners observe seq monotonically by construction. Tradeoff: listener callbacks now run under m.mu, so a listener that calls back into the manager (e.g. getConfig) would deadlock. The existing wrappers AddSessionPoolListener / AddSessionLoadListener and their downstream consumers are self-contained, so this is enforceable as a documented invariant rather than a runtime hazard today. TestManagerNotifyListeners_Race is removed: the race it guarded against — poll() fan-out interleaving with addListener's deferred fire and delivering seq=N+1 before seq=N — is structurally impossible under the new design, and the test's mechanism (block the registration-time fire, then trigger a concurrent poll) now deadlocks rather than exercising anything meaningful.
Keep ClientConfigurationManager and its defaults as standalone pieces in bigtable/internal/transport for this PR; the consumer (Client) and its EnableSession knob will land in the follow-up that actually drives the session pool off the configuration. Without a consumer, starting the poller from NewClientWithConfig only adds an idle RPC loop and an unreachable shutdown path.
The constant only ever has one caller, in the same file, inside an internal/ package. Drop the capital so the public surface of internal/transport doesn't list a knob nobody outside the package can or should touch.
ClientConfiguration.Polling is a oneof; until now the manager only read the PollingConfiguration case and silently ignored StopPolling. The StopPolling bool exists as a server-side backstop against excessive GetClientConfiguration RPCs, so a client that ignores it forces the control plane to choose between living with the load or returning errors instead. Carry StopPolling through as a typed field on pollingConfig, set it in parseConfig from the oneof, and exit pollingLoop at the top of the iteration after a poll that delivers it. The current configuration stays in effect — no fallback to defaults — so the rest of the client keeps using the most recent server-honored config; only further polls are suppressed. Close() still tears the manager down normally.
…ation Address mutianf's review on googleapis#19986: "why do we need to parse the config to go type instead of using the proto object directly?" Drop the parallel Go-struct hierarchy (clientConfig, pollingConfig, sessionConfig, channelPoolConfig, sessionPoolConfig, loadBalancingOptions, channelPoolMode + the Strategy*/mode* constants) and the parseConfig / parsePollingConfig / parseSessionConfig / parseChannelPoolConfig / parseSessionPoolConfig pipeline. ClientConfigurationManager now stores currentConfig, defaultConfig, and lastResponse as *bigtablepb.ClientConfiguration and forwards the raw proto to listeners, so nothing on the read path has to reason about two shapes of the same data. Field-level fallback is done at the accessor site by three small helpers — pollingInterval, validityDuration, maxRPCRetryCount — that read from the proto and reach into defaultClientConfig when the server left the enclosing message out of the oneof / left the field nil. StopPolling and SessionLoad are cheap enough to read via generated getters (cfg.GetStopPolling() / cfg.GetSessionConfiguration(). GetSessionLoad()) so no wrappers for those. Change detection swaps == for proto.Equal; getConfig() and listener fires now return proto.Clone copies so consumers can't race the manager on mutation. Also addressed in the same pass: - The two open-coded time.Hour * 24 * 365 * 100 sentinels (validUntil init in NewClientConfigurationManager, validity-window reset in poll()'s fallback branch) are now derived from validityDuration(defaultClientConfig). One source of truth in the defaults table, no magic literal in the polling code. - AddSessionPoolListener no longer round-trips through a Go struct that dropped Headroom / NewSessionCreationBudget / LoadBalancingOptions / NewSessionCreationPenalty / ConsecutiveSessionFailureThreshold. It now forwards the server's SessionPoolConfiguration verbatim, so downstream PoolSizer / SessionPoolImpl.UpdateConfig see every field the control plane sent. - Additive observability from feat/bigtable-sessionz-debug: PollEvent ring buffer (maxPollHistory=100), lastResponse / lastFetchedAt / lastErr / lastErrAt capture, Snapshot() returning ConfigSnapshot for the configz debug UI. sessionz-only debug-tag calls are skipped — their infra doesn't exist on this branch. defaultClientConfig is now a *bigtablepb.ClientConfiguration proto literal (300s poll / 100y validity / 5 retries / SessionLoad=0 / 2-25 servers / 10 sessions per server / DirectAccessWithFallback with 60s check + 0.8 error threshold / LeastInFlight LB). Values unchanged from the previous Go-struct table. Deferred to follow-up PRs (not addressed here): - mutianf's "pass in a channel instead of BigtableClient" refactor — API surface change. - mutianf's "we might have a server-side version number; leave your monotonic seq out" — semantic listener-contract change. - TelemetryConfiguration handling — no consumer on this branch yet; raw proto passively preserves it in currentConfig for later.
…nfigurationManager mutianf noted on PR googleapis#19986 that the server-side ClientConfiguration proto also carries a TelemetryConfiguration message we currently ignore. Rather than block this PR on designing that surface, leave a TODO next to the existing listener wiring (AddSessionPoolListener / AddSessionLoadListener) so the follow-up author can add AddTelemetryConfigurationListener as a sibling when the telemetry proto stabilizes.
… rewrite from PR googleapis#19986 Bring the proto-native ClientConfigurationManager from the client-config-manager branch (PR googleapis#19986, head a8139aa) onto sessionz. The manager now stores currentConfig, defaultConfig, and lastResponse as *bigtablepb.ClientConfiguration and forwards the raw proto to listeners, so nothing on the read path has to reason about two shapes of the same data. Field-level fallback moves to three small accessors (pollingInterval / validityDuration / maxRPCRetryCount) that reach into defaultClientConfig when the server left the enclosing message out of the oneof. Also folded in from the PR branch: - honor server-requested StopPolling (polling oneof) - drop the *bigtable.Client wire-up in favor of the raw bigtablepb.BigtableClient stub (kills the circular dep future SessionClient would have hit) - collapse shutdown primitives (single cancel + wg, no closeOnce/closed atomic) - close addListener race via the lock, drop monotonicListener - extract fetchClientConfiguration from poll() - unexport MinPollingInterval - validUntil sentinel derived from defaultClientConfig.ValidityDuration, not an open-coded 100-year literal - default_client_config.go split (proto literal, no parallel Go-struct hierarchy) Public API is unchanged — AddSessionPoolListener still hands out *bigtablepb.SessionClientConfiguration_SessionPoolConfiguration, AddSessionLoadListener still hands out float64. Sessionz consumers (session/client.go, session_pool.go, pool_sizer.go) link without changes. TODO(sushanb): plumb TelemetryConfiguration listener. Raised by mutianf on PR googleapis#19986; comment added inline in the manager. Deferred to a follow-up.
🤖 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
ClientConfigurationManagerinbigtable/internal/transport: pollsGetClientConfigurationon a fixed interval, parses the response into a typedclientConfig, and fans changes out to registered listeners. Supports RPC retry, validity-window fallback, andClose()shutdown that waits for in-flight polls.Closesemantics.