Skip to content

feat(bigtable): add ClientConfigurationManager - #19986

Merged
sushanb merged 10 commits into
googleapis:mainfrom
sushanb:client-config-manager
Jul 21, 2026
Merged

feat(bigtable): add ClientConfigurationManager#19986
sushanb merged 10 commits into
googleapis:mainfrom
sushanb:client-config-manager

Conversation

@sushanb

@sushanb sushanb commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add ClientConfigurationManager in bigtable/internal/transport: polls GetClientConfiguration on a fixed interval, parses the response into a typed clientConfig, and fans changes out to registered listeners. Supports RPC retry, validity-window fallback, and Close() shutdown that waits for in-flight polls.
  • Test suite covering construction, polling, listener fan-out, transient-error retry, and Close semantics.

@sushanb
sushanb requested review from a team as code owners June 16, 2026 04:53
@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Jun 16, 2026
@sushanb
sushanb requested review from mutianf and nimf June 16, 2026 04:55

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

Comment thread bigtable/internal/transport/client_configuration_manager.go
Comment thread bigtable/internal/transport/client_configuration_manager.go
Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
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.
@sushanb
sushanb force-pushed the client-config-manager branch from fed66e8 to aedceca Compare June 23, 2026 21:46
@sushanb

sushanb commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Scope reduction — force-pushed to aedceca5bb.

Narrowed this PR down to just the ClientConfigurationManager + the ClientConfig.EnableSession wiring. The session-pool consumers of this manager — clientConfigDirectAccessChecker, CreateSessionChannelPool, the channel_pool_factory.go helper extractions, and the testproxy family-sort drive-by — have been pulled out and will land as a separate follow-up PR for tighter review scope. Mutian's earlier feedback that was specific to those files isn't lost; I'll carry those threads forward when the follow-up opens.

Previous tip (fed66e8dc4) is preserved in my reflog so the dropped pieces can be cherry-picked verbatim into the follow-up branch.

Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
sushanb added 6 commits June 24, 2026 04:19
…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.
@sushanb sushanb changed the title feat(bigtable): add ClientConfigurationManager + EnableSession config feat(bigtable): add ClientConfigurationManager Jun 29, 2026
Comment thread bigtable/internal/transport/client_configuration_manager.go
Comment thread bigtable/internal/transport/client_configuration_manager.go
Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
Comment thread bigtable/internal/transport/client_configuration_manager.go Outdated
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.
sushanb added 2 commits July 21, 2026 05:44
…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.
sushanb added a commit to sushanb/google-cloud-go that referenced this pull request Jul 21, 2026
… 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.
@sushanb
sushanb merged commit 3a8f927 into googleapis:main Jul 21, 2026
19 of 21 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