Skip to content

feat(bigtable): wire Diverter on Client and route Open* via TableShim - #20256

Merged
sushanb merged 9 commits into
googleapis:mainfrom
sushanb:feat/bigtable-client-diverter-open
Jul 29, 2026
Merged

feat(bigtable): wire Diverter on Client and route Open* via TableShim#20256
sushanb merged 9 commits into
googleapis:mainfrom
sushanb:feat/bigtable-client-diverter-open

Conversation

@sushanb

@sushanb sushanb commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

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 *btransport.Diverter, constructed with
    sessionLoad = 0.0 in NewClientWithConfig. Every call stays on
    the classic path until a future change wires the session backend and
    bumps the ratio.

  2. Open / OpenTable / OpenAuthorizedView / OpenMaterializedView
    move out of client.go into a new open.go.
    OpenTable and the
    view variants now return NewTableShim(classic, nil, c.diverter)
    same classic tableImpl inside, routing bolted on. Open()
    (returning *Table) stays classic-only for callers that need the
    concrete pointer type (BulkMutation etc.).

  3. TableShim.pickSession() gate. The session TableAPI is
    optional; with session == nil we short-circuit before
    consulting the diverter — otherwise the pick-count histogram would
    record session picks that got silently downgraded here. ReadRow +
    Apply route through pickSession() instead of calling
    diverter.UseSession() directly.

Also included: MinConns 10 → 4

DefaultDynamicChannelPoolConfig.MinConns drops from 10 to 4. The
session client uses a pool size of 4 as its footprint default (small
enough for the server-driven GetClientConfiguration reshape to take
over quickly); with today's floor of 10 that trips
ValidateDynamicConfig with:

initial connPoolSize (4) must be between DynamicChannelPoolConfig.MinConns (10) and MaxConns (200)

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 WithGRPCConnectionPool land at
defaultBigtableConnPoolSize=4 anyway, so the floor was already
tighter than the actual default in practice.

Behavior

  • Classic path unchanged. sessionLoad = 0.0 makes UseSession()
    return false in every case (its load <= 0 branch is
    short-circuit), so every ReadRow / Apply on a shimmed table
    lands on t.classic.
  • The pickSession() nil-guard is defense-in-depth for the case where
    the session backend isn't wired yet.
  • Diverter.sessionPicks / classicPicks counters start incrementing
    correctly the moment the session backend gets wired in — no debug UI
    needs to move.

Files

File Delta Purpose
bigtable/client.go -54 / -0 net after move Add diverter field; strip Open* methods (moved)
bigtable/open.go +81 new Open / OpenTable / OpenAuthorizedView / OpenMaterializedView
bigtable/table_shim.go +26 / -6 pickSession() nil-safe gate
bigtable/internal/option/option.go +1 / -1 MinConns 10 → 4

Total: +108 / -53 across 4 files.

Test plan

  • go build ./... clean
  • go vet ./... clean
  • go test ./bigtable/... -count=1 -short -timeout=180s — all packages green including bigtable (13s), internal/transport (25s), internal/option (0s), internal/session (0s).

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.
@sushanb
sushanb requested review from a team as code owners July 29, 2026 16:19
@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Jul 29, 2026

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

Comment thread bigtable/table_shim.go Outdated
Comment on lines +50 to +55
func (t *TableShim) pickSession() bool {
if t.session == nil {
return false
}
return t.diverter.UseSession()
}

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.

medium

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.

Suggested change
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()
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
sushanb added a commit that referenced this pull request Jul 29, 2026
…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).
@sushanb

sushanb commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Scope narrowed in 46806045ff — reverted the Table.divertible / applyClassic / readRowClassic / tableImpl.ReadRow bypass / buildDivertible piece from 5b48ef4c26.

After this PR: Client.Open(table string) *Table returns a classic *Table that is NOT session-diverted. OpenTable / OpenAuthorizedView / OpenMaterializedView (returning TableAPI) do route through the TableShim under the Diverter — that's the only entry point that gets divertible routing here.

Making Open divertible without changing its *Table return type (Option B) will follow as its own PR — it's a bigger design conversation (recursion break via tableImpl.ReadRow / applyClassic split) than fits the diverter-wiring scope of this one.

Files changed since the last review pass:

  • bigtable/table.godivertible TableAPI field on Table removed; explicit tableImpl.ReadRow override removed; tableImpl.Apply restored to ti.Table.Apply.
  • bigtable/bigtable.goTable.Apply / Table.ReadRow gate + *Classic split reverted; original bodies restored.
  • bigtable/open.gobuildDivertible helper removed; Open returns bare *Table; OpenTable / OpenAuthorizedView / OpenMaterializedView inline their own &tableImpl{...} + NewTableShim(...) construction (as in the original commit 3fe8b6d403).

The pickSession nil-diverter guard from 53761645a5 stays — it's independent of Option B and defensive regardless.

sushanb added 5 commits July 29, 2026 17:24
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.
@sushanb
sushanb merged commit b32fbd7 into googleapis:main Jul 29, 2026
19 of 21 checks passed
sushanb pushed a commit that referenced this pull request Aug 3, 2026
🤖 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
&lt;resource-id&gt;-&lt;PERM&gt;
([#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>
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