feat(bigtable): add PoolSizer for server-driven session pool capacity - #20189
#20189feat(bigtable): add PoolSizer for server-driven session pool capacity#20189sushanb merged 3 commits intogoogleapis:maingoogleapis/google-cloud-go:mainfrom sushanb:feat/bigtable-pool-sizersushanb/google-cloud-go:feat/bigtable-pool-sizerCopy head branch name to clipboard
Conversation
Adds PoolSizer, a stateless snapshot-driven calculator that produces scale-up / scale-down / dead-band / no-stats decisions from current pool metrics + server config. Standalone (no callers on main yet) so the type + tests land ahead of the SessionPoolImpl consumers. - PoolStats — snapshot of ready/starting/in-use/pending counts. - StatsFetcher — closure indirection so the sizer never reaches into pool internals; the pool passes a getter, the sizer calls it once per Decide. - PoolSizer.Decide — full ScaleDecision trace: every input, every intermediate (EffectivePending, SessionsInUse, IdleHeadroom, DesiredRaw, DesiredCapacity, ImmediateCapacity, EventualCapacity), and the final delta + branch. Operators consume the trace on loadz/sessionz to answer "why did the sizer choose this". - PoolSizer.UpdateConfig — driven by the server-config listener. Guards the same way the constructor does: non-positive Headroom falls back to 0.10; zero NewSessionQueueLength ignored (would divide-by-zero the pending calculation). - Passive-shrink contract: scale-down Delta is advisory only. The client never proactively kills sessions; pool.OnClose reads the delta and lets the pool shrink by one per naturally-closed session. This design cannot oscillate. - MinIdleSessions floor (default 1) prevents cushion-collapse — an idle pool with in-use==0 would otherwise want zero sessions and starve cold-start. - Fail-safe no-stats branch when the fetcher returns nil (pool not started yet). Test coverage: no-stats branch, scale-up from zero, dead-band absorbed by starting, scale-down advisory, MaxSessions clamp, headroom floor, EffectivePending ceil math, headroom default on non-positive (constructor + UpdateConfig), UpdateConfig live-swap, ignore-zero-qlen, trace-fields populated, GetScaleDelta matches Decide, concurrent Decide + UpdateConfig under -race.
…eConfig headroom guard Keeps sessionz-debug in sync with the standalone PoolSizer PR (feat/bigtable-pool-sizer / googleapis#20189): - pool_sizer.go: UpdateConfig now mirrors the constructor's headroomPct <= 0 → 0.10 fallback. Without this, a server configuration carrying Headroom=0 renders as HeadroomPct=0 on the loadz decision trace and collapses IdleHeadroom to the MinIdleSessions floor (pool still works, but the diagnostic trace lies). - pool_sizer_test.go: 13 tests copied verbatim from the PR branch. Covers no-stats branch, scale-up from zero, dead-band absorbed by starting, scale-down advisory, MaxSessions clamp, MinIdleSessions floor, EffectivePending ceil math, headroom default on non-positive (constructor + UpdateConfig), UpdateConfig live-swap, ignore-zero-qlen, trace-fields populated, GetScaleDelta matches Decide, concurrent Decide + UpdateConfig under -race. Full transport suite green under -race -short.
There was a problem hiding this comment.
Code Review
This pull request introduces the PoolSizer utility and its corresponding tests to dynamically calculate the optimal session pool size for Bigtable based on workload metrics. The review feedback highlights three key improvement opportunities: adding a defensive nil check for the configuration in UpdateConfig to prevent potential panics, invoking the external fetcher callback outside of the mutex lock in Decide to avoid deadlocks and nil pointer dereferences, and utilizing idiomatic integer arithmetic instead of floating-point conversions with math.Ceil for calculating effective pending sessions.
| func (s *PoolSizer) UpdateConfig(config *spb.SessionClientConfiguration_SessionPoolConfiguration) { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() |
There was a problem hiding this comment.
If config is nil, accessing its fields (like config.MinSessionCount) will cause a nil pointer dereference panic. Adding a defensive nil check at the beginning of UpdateConfig prevents this.
| func (s *PoolSizer) UpdateConfig(config *spb.SessionClientConfiguration_SessionPoolConfiguration) { | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| func (s *PoolSizer) UpdateConfig(config *spb.SessionClientConfiguration_SessionPoolConfiguration) { | |
| if config == nil { | |
| return | |
| } | |
| s.mu.Lock() | |
| defer s.mu.Unlock() |
There was a problem hiding this comment.
Fixed in c4790d1 — early return on nil config. Added TestPoolSizer_UpdateConfigNilNoOp to lock it in.
| func (s *PoolSizer) Decide() ScaleDecision { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| d := ScaleDecision{ | ||
| MinSessions: s.minSessions, | ||
| MaxSessions: s.maxSessions, | ||
| HeadroomPct: s.headroomPct, | ||
| NewSessionQLen: s.newSessionQLen, | ||
| MinIdleSessions: s.minIdleSessions, | ||
| } | ||
|
|
||
| stats := s.fetcher() | ||
| if stats == nil { | ||
| d.Branch = "no-stats" | ||
| return d | ||
| } |
There was a problem hiding this comment.
Calling the external fetcher callback while holding the internal mutex s.mu can lead to potential deadlocks or lock inversion if the callback implementation attempts to acquire locks or call back into the pool sizer. Additionally, if s.fetcher is nil, calling it directly will cause a panic.
We can safely retrieve the stats by calling the fetcher before acquiring the lock, as s.fetcher is read-only after construction. This also allows us to gracefully handle a nil fetcher.
func (s *PoolSizer) Decide() ScaleDecision {
var stats *PoolStats
if s.fetcher != nil {
stats = s.fetcher()
}
s.mu.Lock()
defer s.mu.Unlock()
d := ScaleDecision{
MinSessions: s.minSessions,
MaxSessions: s.maxSessions,
HeadroomPct: s.headroomPct,
NewSessionQLen: s.newSessionQLen,
MinIdleSessions: s.minIdleSessions,
}
if stats == nil {
d.Branch = "no-stats"
return d
}There was a problem hiding this comment.
Fixed in c4790d1. Hoisted stats := s.fetcher() above s.mu.Lock() — s.fetcher is set once at construction so the lock-free read races nothing, and no sizer lock is held across the callback anymore. Nil-fetcher path returns no-stats cleanly. TestPoolSizer_DecideFetcherOutsideLock pins the invariant with a 2s deadlock timeout; TestPoolSizer_DecideNilFetcher covers the nil case.
| if divisor <= 0 { | ||
| divisor = defaultNewSessionQueueLength | ||
| } | ||
| d.EffectivePending = int(math.Ceil(float64(stats.PendingCount) / float64(divisor))) |
There was a problem hiding this comment.
For non-negative integer ceiling division, we can use pure integer arithmetic (A + B - 1) / B instead of converting to float64 and calling math.Ceil. This is more idiomatic in Go, avoids float conversions, and is slightly more efficient.
| d.EffectivePending = int(math.Ceil(float64(stats.PendingCount) / float64(divisor))) | |
| d.EffectivePending = (stats.PendingCount + divisor - 1) / divisor |
There was a problem hiding this comment.
Fixed in c4790d1 — EffectivePending = (stats.PendingCount + divisor - 1) / divisor. math import retained for IdleHeadroom which multiplies by a float fraction (integer-ceil trick doesnt apply cleanly there); comment updated to explain. TestPoolSizer_EffectivePendingIntegerArithmeticExhaustive compares against math.Ceil across 0..100 pending × 1..20 qlen (2020-cell grid) to guarantee identical results in the production domain.
Three inline comments on PR googleapis#20189: 1. UpdateConfig(nil) would panic. Add early return; comment notes the defensive intent for startup intermediates from the listener path. 2. Decide called s.fetcher() while holding s.mu. If a future fetcher takes the pool's mutex, that becomes a lock-inversion trap. Hoist the fetcher call above s.mu.Lock(); safe because s.fetcher is set-once at construction. Nil fetcher gracefully returns no-stats. 3. EffectivePending used int(math.Ceil(float64(a)/float64(b))). For non-negative a and positive b, (a+b-1)/b is the idiomatic Go integer ceiling — no float conversion, no allocation. math import retained for IdleHeadroom which multiplies by a float fraction where the integer trick doesn't apply. Four new tests: - TestPoolSizer_UpdateConfigNilNoOp — nil config is a no-op, not a panic. - TestPoolSizer_DecideNilFetcher — nil fetcher returns no-stats cleanly. - TestPoolSizer_DecideFetcherOutsideLock — fetcher runs unlocked; 2s deadlock-detection timeout on a goroutine. - TestPoolSizer_EffectivePendingIntegerArithmeticExhaustive — the integer formula matches math.Ceil across 0..100 pending × 1..20 qlen (2020-cell grid).
|
Addressed all three review comments in c4790d1:
18 tests pass under |
…4790d1) Keeps sessionz-debug in sync with the upstream-facing pool-sizer branch. Three fixes address gemini-code-assist comments on PR googleapis#20189: 1. UpdateConfig(nil) guard — early return instead of nil-pointer panic. Defensive for startup intermediates from the listener path. 2. Fetcher hoisted outside s.mu — Decide called s.fetcher() while holding the sizer's mutex; if a future fetcher takes the pool's mutex, that becomes a lock-inversion trap. s.fetcher is set-once at construction so the lock-free read races nothing. Nil fetcher gracefully returns no-stats. 3. EffectivePending uses integer ceil (a+b-1)/b instead of int(math.Ceil(float64/float64)) — idiomatic Go, no float conversion. math import retained for IdleHeadroom which multiplies by a float fraction where the integer trick doesn't apply. Four new tests port verbatim: - TestPoolSizer_UpdateConfigNilNoOp - TestPoolSizer_DecideNilFetcher - TestPoolSizer_DecideFetcherOutsideLock (2s deadlock-detection timeout) - TestPoolSizer_EffectivePendingIntegerArithmeticExhaustive Full transport suite green under -race -short.
| // config. | ||
| func NewPoolSizer(fetcher StatsFetcher, minSessions, maxSessions int, headroomPct float64) *PoolSizer { | ||
| if headroomPct <= 0 { | ||
| headroomPct = 0.10 |
There was a problem hiding this comment.
nits: let's keep all the client configuration default into a bigtable-default-client-config.textproto file, instead of hard coded and scattered around different classes :), similar to https://github.com/googleapis/google-cloud-java/blob/main/java-bigtable/google-cloud-bigtable/src/main/resources/bigtable-default-client-config.textproto
There was a problem hiding this comment.
Applied in a716de4 — but by pointing pool_sizer at the existing defaultClientConfig (proto-typed defaults in bigtable/internal/transport/default_client_config.go) rather than adding a new .textproto.
NewPoolSizer and UpdateConfig now read Headroom / NewSessionQueueLength / min-max-session defaults from defaultClientConfig.GetSessionConfiguration().GetSessionPoolConfiguration() — same source ClientConfigurationManager already uses. Only defaultMinIdleSessions=1 stays as a client-side constant (no matching proto field; purely a floor to prevent ceil(0 * headroom)==0 starvation).
If you want to go one step further and lift default_client_config.go itself into a checked-in .textproto loaded via prototext.Unmarshal at init (fuller Java parity), that would be a separate refactor spanning default_client_config.go + client_configuration_manager.go + build-time embed.FS. Happy to spin that up as a follow-up PR if that direction is preferred.
…ientConfig Addresses mutianf's review on PR googleapis#20189: "let's keep all the client configuration defaults in one place, instead of hard coded and scattered around different classes." The Go client already has defaultClientConfig (a live *ClientConfiguration proto in default_client_config.go) — the server-shipped SessionPoolConfiguration lives at defaultClientConfig.GetSessionConfiguration().GetSessionPoolConfiguration() and carries Headroom=0.5, NewSessionQueueLength=10, MinSessionCount=5, MaxSessionCount=400. pool_sizer.go now reads its fallback defaults from that proto: - NewPoolSizer's `headroomPct <= 0` branch reads Headroom (was hardcoded 0.10; now picks up 0.5 which matches the server default). - UpdateConfig's `Headroom <= 0` guard reads the same source. - Decide's divide-by-zero guard reads NewSessionQueueLength (still 10, but not hardcoded). defaultMinIdleSessions=1 stays as a client-side constant — no proto field carries it (purely a floor to prevent ceil(0*headroom)==0 starvation). Test updates: - TestPoolSizer_HeadroomDefaultOnNonPositive reads the expected value from defaultPoolConfig() instead of hardcoding 0.10. - TestPoolSizer_UpdateConfigNormalizesNonPositiveHeadroom same. - TestPoolSizer_EffectivePendingCeil simplified (no gate around s.newSessionQLen assignment now that the const is gone). 18 tests pass under -race.
…oogleapis#20189 (a716de4) Keeps sessionz in sync with the upstream-facing pool-sizer branch. Addresses mutianf's review comment about centralizing client configuration defaults: pool_sizer now reads its fallback Headroom / NewSessionQueueLength values from the existing defaultClientConfig proto in default_client_config.go instead of hardcoding 0.10 / 10 in NewPoolSizer, UpdateConfig, and Decide's divisor-guard. defaultMinIdleSessions=1 stays as a client-side constant — no matching proto field (purely a floor to prevent ceil(0 * headroom)==0 starvation). Tests updated to read the expected value from defaultPoolConfig() rather than hardcoding the old 0.10. All 18 pool_sizer tests pass under -race; full transport suite green under -race -short (40s).
🤖 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
Adds
PoolSizer, a stateless snapshot-driven calculator that produces scale-up / scale-down / dead-band / no-stats decisions from current pool metrics + server config. Standalone (no callers on main yet) so the type + tests can land ahead of the upcomingSessionPoolImplconsumers.PoolStats— snapshot of ready / starting / in-use / pending counts at a single instant.StatsFetcher— closure indirection so the sizer never reaches into pool internals; the pool passes a getter, the sizer calls it once perDecide.PoolSizer.Decide— returns a fullScaleDecisiontrace: every input, every intermediate (EffectivePending,SessionsInUse,IdleHeadroom,DesiredRaw,DesiredCapacity,ImmediateCapacity,EventualCapacity), and the finalDelta+Branch. Operators consume the trace on debug pages to answer "why did the sizer choose this" without re-running the arithmetic.PoolSizer.UpdateConfig— driven by the server-config listener. Guards the same way the constructor does: non-positiveHeadroomfalls back to0.10; zeroNewSessionQueueLengthis ignored (would divide-by-zero the pending calculation).Deltais advisory only. The client never proactively kills sessions; the pool'sOnClosereads the delta and lets the pool shrink by one per naturally-closed session. This design cannot oscillate.MinIdleSessionsfloor (default 1) prevents cushion-collapse — an idle pool withInUseCount==0would otherwise want zero sessions and starve cold-start.no-statsbranch when the fetcher returns nil (pool not started yet).Test plan
go build ./bigtable/...go test ./bigtable/internal/transport/ -run '^TestPoolSizer' -count=1 -race— 13 tests pass (1.0s)gofmt -l bigtable/internal/transport/pool_sizer*.go— cleango vet ./bigtable/internal/transport/— cleanTest coverage:
no-statsbranch on nil fetcherMinSessions)MaxSessionsclamp under extreme demandMinIdleSessionsfloor prevents cushion-collapseEffectivePending = ceil(PendingCount / NewSessionQueueLength)(table-driven)UpdateConfigboth normalize non-positiveHeadroomPctto0.10UpdateConfiglive-swap takes effectNewSessionQueueLengthfrom server ignored (divide-by-zero guard)ScaleDecisionintermediate populated from one evaluationGetScaleDeltamatchesDecide().DeltaDecide+UpdateConfigunder-race(single mutex)