Repository navigation
fix(bigtable): keep single-key ReadRows on the streaming classic path - #20506
Merged
mutianf merged 1 commit intoSep 10, 2026
Merged
Conversation
product-auto-label
Bot
added
the
api: bigtable
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the Bigtable client's read path to ensure that the classic fallback for ReadRows always routes to classic.ReadRows rather than classic.ReadRow. This change preserves the streaming deliver-then-error contract (preventing rows from being dropped on stream failures) and resolves conformance test failures. It also updates the documentation and adds corresponding unit tests to verify the routing behavior. I have no feedback to provide as there are no review comments.
mutianf
changed the title
mutianf
force-pushed
the
fix-bigtable-single-key-readrows
branch
from
September 10, 2026 03:03
097f324 to
340a34c
Compare
mutianf
changed the title
mutianf
force-pushed
the
fix-bigtable-single-key-readrows
branch
from
September 10, 2026 03:14
340a34c to
29a6741
Compare
googleapis#20432 taught TableShim.ReadRows to detect a single-row RowSet and dispatch it through TableShim.ReadRow. But ReadRow buffers — it returns (Row, error) — while ReadRows streams, invoking the callback as rows arrive and returning the error afterwards. The shim collapsed that into: row, err := t.ReadRow(ctx, key, opts...) if err != nil { return err } // rows already streamed are dropped readRows hands each row to f inline (bigtable.go:322) and can then return an unretryable error from cr.Process or cr.Close(), so a row delivered before a terminal stream error was silently discarded. This is not test-only wiring: client.go sets diverter unconditionally, so Open always populates t.divertible and every ReadRows(SingleRow(k)) went through the shim — even at SessionLoad=0.0 with no session backend. The ReadRows conformance suite catches it. "invalid - last row missing commit" streams a complete RK_1 then a truncated RK_2; classic yields RK_1 and then errors, the buffered path yielded nothing. Eleven cases failed in total; the "two rows*" cluster is amplified by the conformance mock ignoring the rows_limit=1 that readRowClassic injects, but the dropped-row-before-error case is a real regression. Fix: TableShim.ReadRows checks useSession() up front, and every branch not actually served by the session plane — diverter off, session unavailable, Unimplemented breaker tripped, or an Unimplemented response — goes to classic.ReadRows, never classic.ReadRow. Extracted readRowSession so ReadRow and the single-key ReadRows branch build the same request while each owns its own fallback target. Replace InterceptUnimplemented/InterceptUnimplementedErr with UnimplementedBreaker.ShouldFallback(err). The wrappers forced every call site into func() (T, error), which single-key ReadRows cannot be — its result leaves through a callback, so that site had to thread a struct{}{} through a generic just to discard it. A predicate lets each call site keep its own shape, and folding the counter bookkeeping into it means routing can't record one verdict and then act on another. The type is renamed from UnimplementedErrorInterceptor since it no longer intercepts. Also point Table.readRowClassic at readRowsClassic instead of the divertible-gated Table.ReadRows. It issues SingleRow(row), exactly the diverted shape, so a Table with divertible set could bounce a classic read back into the shim and on into TableShim.ReadRow. Production nils divertible on the inner snapshot (open.go:56) so this never fired, but TestTableImpl_BypassesDivertibleGate was already red on the invariant. Two known gaps left alone: WithFullReadStats does nothing on the diverted path (SessionReadRowRequest carries only Key and Filter, and session has no read stats today), and the breaker trip is sticky where Java's recovers — Java counts Unimplemented on session open, which keeps running while the gate is shut. Fixing the latter means moving the counter into the session pool; filed as a TODO on UnimplementedBreaker. Tests: two shim tests asserted the buggy routing (classic.readRowCalls == 1); rewritten to pin classic.ReadRows, plus a regression test that a row streamed before an error still reaches the callback. TestOpen_ClassicOnlyMethodsSkipDivertibleGate claimed ReadRows must not gate on divertible — stale since googleapis#20432, so ReadRows is dropped from it and TestOpen_ReadRowsGatesOnDivertible covers the real invariant. New unimplemented_breaker_test.go covers the verdict per error class, the count reset, and the sticky trip. Change-Id: I031ed81d732205d50f7f09c9237e17e62cf0094e
mutianf
force-pushed
the
fix-bigtable-single-key-readrows
branch
from
September 10, 2026 03:21
29a6741 to
08bf4cf
Compare
sushanb
approved these changes
Sep 10, 2026
sushanb
approved these changes
Sep 10, 2026
dbolduc
pushed a commit
that referenced
this pull request
Sep 10, 2026
🤖 I have created a release *beep* *boop* --- ## [1.57.0](bigtable/v1.56.0...bigtable/v1.57.0) (2026-09-10) ### Features * **bigtable:** Build the accelerator daemon for Windows ([#20455](#20455)) ([df55af5](df55af5)) ### Bug Fixes * **bigtable/bttest:** Honor timestamp_origin on SetCell ([#20469](#20469)) ([528aaf8](528aaf8)), refs [#20468](#20468) * **bigtable:** Keep single-key ReadRows on the streaming classic path ([#20506](#20506)) ([2bfdb5d](2bfdb5d)) --- 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix conformance test + clean up unimplemented interceptor.
ReadRows could return a row followed by an error. Routing readRow through readRows broke this contract.