feat(bigtable): wire Diverter on Client and route Open* via TableShim - #20256
Conversation
Three coordinated changes so a follow-up session-data-path patch can flip traffic through the shim without further Client/OpenTable churn: 1. Client owns a *transport.Diverter, constructed with sessionLoad=0.0 in NewClientWithConfig. Every call stays on the classic path until a future change wires in the session backend and bumps the ratio. 2. Open / OpenTable / OpenAuthorizedView / OpenMaterializedView move out of client.go into a new open.go. OpenTable & friends now return NewTableShim(classic, nil, c.diverter) — the shim wraps the same classic tableImpl as before with routing added on top. Open() (returning *Table) stays classic-only for callers that need the concrete pointer type. 3. TableShim gains a pickSession() gate: session TableAPI is optional; with session==nil pickSession short-circuits before consulting the diverter so the pick-count histogram doesn't record session picks that got silently downgraded here. ReadRow + Apply route through pickSession() instead of calling diverter.UseSession() directly. Also: DefaultDynamicChannelPoolConfig.MinConns 10 -> 4. Session client uses a pool of 4 as its footprint default, and today ValidateDynamicConfig rejects that with "initial connPoolSize (4) must be between MinConns (10) and MaxConns (200)". Lowering the floor to 4 lets the session pool validate under the same default config the classic pool uses. No behavior change on the classic path: sessionLoad=0.0 makes UseSession() return false in every case, and the pickSession() nil-guard would defend even if it didn't. All existing bigtable tests pass.
There was a problem hiding this comment.
Code Review
This pull request introduces a Diverter to the Bigtable client to route traffic between the classic and future session data paths, moving the Open* methods to a new bigtable/open.go file and wrapping their returns in a TableShim. It also reduces the default minimum connections in DefaultDynamicChannelPoolConfig from 10 to 4. The feedback suggests adding a nil check for t.diverter in pickSession() to prevent potential nil pointer dereference panics if TableShim is instantiated with a nil diverter.
| func (t *TableShim) pickSession() bool { | ||
| if t.session == nil { | ||
| return false | ||
| } | ||
| return t.diverter.UseSession() | ||
| } |
There was a problem hiding this comment.
Since TableShim is an exported struct, it can be instantiated directly by external callers or in tests without initializing the diverter field. Additionally, if the Client is mocked or constructed in a way where diverter is nil, calling t.diverter.UseSession() will cause a nil pointer dereference panic. Adding a nil check for t.diverter in pickSession() ensures safe fallback to the classic path.
| func (t *TableShim) pickSession() bool { | |
| if t.session == nil { | |
| return false | |
| } | |
| return t.diverter.UseSession() | |
| } | |
| func (t *TableShim) pickSession() bool { | |
| if t.session == nil || t.diverter == nil { | |
| return false | |
| } | |
| return t.diverter.UseSession() | |
| } |
There was a problem hiding this comment.
Fixed in 5376164 — widened the short-circuit to t.session == nil || t.diverter == nil. Nil-session guard still runs first (so pick-count telemetry isn't inflated by session picks that got downgraded here).
Follow-up on the diverter wiring in this PR. Makes Client.Open route
through the same TableShim path that OpenTable / OpenAuthorizedView /
OpenMaterializedView already use, without changing Open's *Table
return type — so classic callers that hold a concrete *Table pointer
(BulkMutation and any external code) will get session routing when the
session backend lands in a follow-up PR.
Wiring:
- Table gains a divertible TableAPI field. Nil for classic-only clients.
- Table.Apply / Table.ReadRow become one-line gates:
if t.divertible != nil { return t.divertible.X(...) }
return t.xClassic(...)
- Table.applyClassic / Table.readRowClassic hold the original bodies
verbatim — no metrics / tracer / retry behavior change on classic.
- tableImpl grows an explicit ReadRow override; both tableImpl.Apply
and tableImpl.ReadRow now call the *Classic helpers directly,
bypassing the gate. This breaks the recursion loop: the shim's
classic branch dispatches into tableImpl, which lands on applyClassic
/ readRowClassic instead of coming back through Table.Apply /
Table.ReadRow.
- Open populates t.divertible via a new buildDivertible helper that
constructs NewTableShim(&tableImpl{Table: <snapshot with divertible
zeroed>}, nil, c.diverter) when c.diverter != nil. The
snapshot-with-divertible-zeroed is defense in depth against a future
reader who removes the tableImpl overrides.
- OpenTable / OpenAuthorizedView / OpenMaterializedView refactored to
reuse buildDivertible so all four Open* methods construct the shim
identically. Session TableAPI is nil today; the follow-up wires it.
Behavioral guarantees:
- Conditional Apply (CheckAndMutateRow) still routes to classic —
TableShim.Apply enforces that on the shim side; the Table-level gate
is unconditional so every conditional passes through the shim, which
re-routes it to classic. Same end state.
- ReadRows / SampleRowKeys / ApplyBulk / ApplyReadModifyWrite are
untouched — no session equivalent, no gate needed.
- Classic-only clients (c.diverter == nil) short-circuit at
buildDivertible; Table.divertible stays nil; both gates fall through
to *Classic on the fast path. Zero perf change.
All target tests green.
gemini review on PR googleapis#20256 flagged that pickSession would NPE if a caller built a TableShim without a diverter (TableShim is exported, so this is reachable from tests and external code even though in-repo callers always wire one via NewTableShim). Widens the short-circuit to `t.session == nil || t.diverter == nil`. Nil-session guard still runs FIRST — same reason as before: consulting the diverter is side-effectful (per-outcome pick counters), and we don't want session picks that got downgraded to classic here inflating the histogram.
…20258) ## Summary Follow-up to #20257 (protoRowToRow), which now has its sole consumer. Swaps `TableShim`'s session backend from the classic `TableAPI` shape (row string + `bigtable.Row`) to the internal `session.TableAPI` shape (proto-native `*btpb.SessionReadRow{Request,Response}` + `*btpb.SessionMutateRow{Request,Response}`). `TableShim` now owns the proto ↔ public-types translation so the `internal/session` package can stay proto-native. ## Behavior **`ReadRow`** — parses `ReadOption` using the classic `makeReadSettings` shape (so filter + full-read-stats callback plumbing stays in one place), builds a `*btpb.SessionReadRowRequest`, calls `session.ReadRow`, feeds any `resp.Stats` through the `WithFullReadStats` callback, and converts `resp.Row` via `protoRowToRow`. **`Apply`** — conditional mutations (`CheckAndMutateRow`) always route to classic since the session vRPC has no `CheckAndMutateRow` equivalent. Non-conditional mutations build a `*btpb.SessionMutateRowRequest` and call `session.MutateRow`. **`useSession()`** — nil-safe on both `session` and `diverter`, so callers can wire a `TableShim` with `nil` session (as `buildDivertible` does in #20256) and every routing decision falls through to classic. **`ReadRows` / `SampleRowKeys` / `ApplyBulk` / `ApplyReadModifyWrite` always classic** — no session equivalent in the vRPC. ## API-visible signature change `NewTableShim`'s `session` parameter type becomes `session.TableAPI` (an internal-package interface). Existing in-repo callers pass `nil` for session today; nil is the zero value of any interface, so the change is source-compatible for those. External callers writing tests can mock `session.TableAPI` directly — the new test file has an example (`mockSessionTable`). ## Tests Replaces the `mockTableAPI`-as-session pattern with a proto-native `mockSessionTable`. New coverage: - `TestTableShim_ReadRow_RoutesByDiverter` — classic when `SessionLoad=0.0`; session when `SessionLoad=1.0`; classic fallback when session is nil even with `SessionLoad=1.0`; session error propagation (no automatic fallback to classic on failure). - `TestTableShim_Apply_ConditionalAlwaysClassic` — pins that conditional mutations bypass the session path. - `TestTableShim_Apply_NonConditionalRoutesByDiverter` — pins that non-conditional mutations follow the diverter. - `TestTableShim_ReadRows_AlwaysClassic`, `TestTableShim_SampleRowKeys_AlwaysClassic`, `TestTableShim_ApplyBulk_AlwaysClassic`, `TestTableShim_ApplyReadModifyWrite_AlwaysClassic` — pin the no-session-equivalent methods. - `TestTableShim_NilSession_AllMethodsFallBackToClassic` — the classic-only wiring path (what #20256's `buildDivertible` will use until the session backend lands). - `TestTableShim_SessionErrorNotRetriedOnClassic` — session-side failures surface as-is instead of silently falling back. ## Test plan - [x] `go build ./...` clean - [x] `go vet ./...` clean - [x] `go test ./bigtable/ -run 'TestTableShim|TestProtoRowToRow' -count=1 -v` — all pass - [x] Live sandbox smoke against `autonomous-mote-782 / sushanb-uc1` — classic path via `client.Open(...).Apply/ReadRow` still succeeds end-to-end (session backend not exercised on this branch since it isn't wired yet) ## Depends on - #20257 (merged as `1297143a4f`) — `protoRowToRow` helper. ## Follows Consumed by a future PR that wires an actual `session.TableAPI` implementation from the `internal/session` package into `Client.buildDivertible` (see #20256 for the `buildDivertible` shape).
…ableShim" This reverts commit 5b48ef4.
|
Scope narrowed in After this PR: Making Files changed since the last review pass:
The |
Folds PR googleapis#20258 into this PR. TableShim's session backend changes from the classic-shaped TableAPI (row string + bigtable.Row) to the internal/session.TableAPI shape (proto-native *btpb.SessionReadRow{Request,Response} + *btpb.SessionMutateRow {Request,Response}). TableShim owns the proto ↔ public-types translation so the session package can stay proto-native. - ReadRow: parses ReadOptions via classic makeReadSettings/readSettings so every existing option (RowFilter, WithFullReadStats, etc.) works on the session path; then translates (row, filter) → SessionReadRowRequest, calls session.ReadRow, feeds resp.Stats through WithFullReadStats, and converts resp.Row via protoRowToRow (added in googleapis#20257, now on main). - Apply: nil mutations and conditional mutations (CheckAndMutateRow) route to classic — CheckAndMutateRow has no session vRPC equivalent, and nil-m to classic surfaces the classic client's existing nil-handling error rather than panicking on m.ops here. - useSession() helper is nil-safe on both session and diverter (replaces the earlier pickSession); callers can wire a TableShim with nil session and every routing decision falls through to classic. Signature change (internal-only): NewTableShim's session parameter type is now session.TableAPI (an internal package). In-repo callers pass nil for session today; nil is the zero value of any interface, so the change is source-compatible for those. Tests: replaces the mockTableAPI-as-session pattern with a proto-native mockSessionTable. Covers routing, nil-session fallback, session error propagation, conditional-always-classic, and the already-existing TestProtoRowToRow contract. Closes PR googleapis#20258 (the standalone version); folded here so PR googleapis#20256's review scope is one coherent unit — Diverter wiring + TableShim proto-native shape together.
goimports flagged bigtable/client.go on CI. The Option B revert left a double blank line where the four Open* methods used to sit; goimports canonicalizes that to a single blank line. Pure whitespace fix.
Adds bigtable/open_test.go with 5 tests pinning the factory contract introduced by this PR. Focused on the composition (which shape each factory returns + how it wires the client's Diverter) rather than re-testing routing behavior — TableShim's own tests already cover nil-session-routes-to-classic (TestTableShim_NilSession_AllMethodsFallBackToClassic on main). Tests: - TestOpen_ReturnsBareTable — Open returns plain *Table (no session-routing wrapper). Callers holding *Table stay on the classic path regardless of Client's diverter setting. - TestOpenTable_ProducesNilSessionShim — OpenTable returns *TableShim with session == nil and classic == *tableImpl. Uses SessionLoad=1.0 on the diverter to prove useSession() still returns false because session is nil (the nil-session short-circuit runs before consulting the diverter). - TestOpenAuthorizedView_ProducesNilSessionShim — same, plus authorizedView threaded through to the inner Table. - TestOpenMaterializedView_ProducesNilSessionShim — same, plus materializedView threaded through (table empty since MVs are addressed by view name only). - TestOpenFactories_ShareOneClientDiverter — every Open* factory references the SAME *Diverter, and mutating it via SetSessionLoad is visible from every shim (proves the reference is live, not a copy). This is the invariant a future ConfigurationManager.SetSessionLoad depends on to update every open resource at once. Test bootstrap uses a hand-built minimal *Client (project + appProfile + featureFlagsMD + diverter) rather than NewClientWithConfig to keep the tests dial-free and hermetic — the factory code paths only read those fields.
🤖 I have created a release *beep* *boop* --- ## [1.52.0](bigtable/v1.51.0...bigtable/v1.52.0) (2026-08-03) ### Features * **bigtable:** Add AFE picker (Simple / LeastInFlight / LeastLatency) ([#20204](#20204)) ([bcbf714](bcbf714)) * **bigtable:** Add ClientConfig.DisableSession to opt out of session backend ([#20297](#20297)) ([7ee5e44](7ee5e44)) * **bigtable:** Add getClientConfigDirectAccessChecker for session pools ([#20209](#20209)) ([3b8d30a](3b8d30a)) * **bigtable:** Add NoOpChannelPrimer for session channel pools ([#20208](#20208)) ([d055a8a](d055a8a)) * **bigtable:** Add per-AFE sessionList for the two-tier session pool ([#20224](#20224)) ([dbf0c3f](dbf0c3f)) * **bigtable:** Add protoRowToRow conversion helper for TableShim ([#20257](#20257)) ([1297143](1297143)) * **bigtable:** Add Session debug surface (observability fields + methods) ([#20211](#20211)) ([d8d3e16](d8d3e16)) * **bigtable:** Add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) ([#20215](#20215)) ([b9e53c6](b9e53c6)) * **bigtable:** Add Session struct + state machine ([#20117](#20117)) ([09acbb3](09acbb3)) * **bigtable:** Add session.Config.EnableDebug to gate sessionz debug state ([#20247](#20247)) ([ce74c31](ce74c31)) * **bigtable:** Add SessionClient + SessionTable + lazyPool ([#20228](#20228)) ([ab2c96c](ab2c96c)) * **bigtable:** Add SessionPoolImpl (two-tier pool + scaling + debug) ([#20225](#20225)) ([683eda8](683eda8)) * **bigtable:** Rename session pool display to <resource-id>-<PERM> ([#20248](#20248)) ([35e146e](35e146e)) * **bigtable:** Route Client.Open()-returned *Table through the Diverter ([#20273](#20273)) ([2b81c7d](2b81c7d)) * **bigtable:** State-based classification for abnormal session close ([#20243](#20243)) ([f2905b7](f2905b7)) * **bigtable:** TableShim fallback to classic on session UNIMPLEMENTED ([#20269](#20269)) ([36540af](36540af)) * **bigtable:** TTL-on-idle cache for per-resource session.TableAPI ([#20263](#20263)) ([00b2a49](00b2a49)) * **bigtable:** Wire Diverter on Client and route Open* via TableShim ([#20256](#20256)) ([b32fbd7](b32fbd7)) ### Bug Fixes * **bigtable:** AFE picker latency signal — subtract poolWait and compute TransportLatency = wire − backend at source ([#20281](#20281)) ([bb8c4d5](bb8c4d5)) * **bigtable:** Guard NewStream OnFinish against grpc-go double-fire ([#20295](#20295)) ([b51da29](b51da29)) * **bigtable:** Real per-resource pool teardown on sessionTable.Close + cache close-race gate ([#20264](#20264)) ([599aea9](599aea9)) * **bigtable:** Session.durations / session.uptime — set explicit histogram bucket boundaries ([#20276](#20276)) ([97eee22](97eee22)) * **bigtable:** SessionTableHandle self-heals across cache eviction ([#20296](#20296)) ([0dd98cd](0dd98cd)) * **bigtable:** Translate ctx errors to gRPC status on session vRPC ([#20299](#20299)) ([0f3b2a5](0f3b2a5)) * **bigtable:** Treat PingAndWarm NotFound as a successful prime ([#20219](#20219)) ([a1557ad](a1557ad)) ### Performance Improvements * **bigtable:** Delete periodic Tick loop; sizing is event-driven ([#20285](#20285)) ([2c096bd](2c096bd)) * **bigtable:** Drop pick_lost_race debug tag from CheckoutSession hot path ([#20280](#20280)) ([bd0e400](bd0e400)) --- 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
Three coordinated changes so a follow-up session-data-path patch can
flip traffic through the shim without further Client / OpenTable churn:
Client owns a
*btransport.Diverter, constructed withsessionLoad = 0.0inNewClientWithConfig. Every call stays onthe classic path until a future change wires the session backend and
bumps the ratio.
Open/OpenTable/OpenAuthorizedView/OpenMaterializedViewmove out of
client.gointo a newopen.go.OpenTableand theview variants now return
NewTableShim(classic, nil, c.diverter)—same classic
tableImplinside, routing bolted on.Open()(returning
*Table) stays classic-only for callers that need theconcrete pointer type (BulkMutation etc.).
TableShim.pickSession()gate. The sessionTableAPIisoptional; with
session == nilwe short-circuit beforeconsulting the diverter — otherwise the pick-count histogram would
record session picks that got silently downgraded here.
ReadRow+Applyroute throughpickSession()instead of callingdiverter.UseSession()directly.Also included:
MinConns10 → 4DefaultDynamicChannelPoolConfig.MinConnsdrops from 10 to 4. Thesession client uses a pool size of 4 as its footprint default (small
enough for the server-driven
GetClientConfigurationreshape to takeover quickly); with today's floor of 10 that trips
ValidateDynamicConfigwith:Lowering the floor lets the session pool validate under the same
default config the classic pool uses. No effect on the classic path:
classic clients that don't call
WithGRPCConnectionPoolland atdefaultBigtableConnPoolSize=4anyway, so the floor was alreadytighter than the actual default in practice.
Behavior
sessionLoad = 0.0makesUseSession()return
falsein every case (itsload <= 0branch isshort-circuit), so every
ReadRow/Applyon a shimmed tablelands on
t.classic.pickSession()nil-guard is defense-in-depth for the case wherethe session backend isn't wired yet.
Diverter.sessionPicks/classicPickscounters start incrementingcorrectly the moment the session backend gets wired in — no debug UI
needs to move.
Files
bigtable/client.godiverterfield; stripOpen*methods (moved)bigtable/open.goOpen/OpenTable/OpenAuthorizedView/OpenMaterializedViewbigtable/table_shim.gopickSession()nil-safe gatebigtable/internal/option/option.goMinConns10 → 4Total: +108 / -53 across 4 files.
Test plan
go build ./...cleango vet ./...cleango test ./bigtable/... -count=1 -short -timeout=180s— all packages green includingbigtable(13s),internal/transport(25s),internal/option(0s),internal/session(0s).