Repository navigation
feat(bigtable): support SessionReadModifyWriteRow on the session client - #14585
Merged
Merged
Conversation
![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 introduces support for ReadModifyWriteRow operations in the session-based data path of the Bigtable client. It adds the necessary request/response handling, descriptors, and a new ReadModifyWriteRowShim class, along with comprehensive unit tests. The feedback recommends removing an obsolete TODO comment in TableAsync.java regarding retrieving the deadline from the compatibility layer, as this is already successfully implemented.
…lowing ReadRowShim pattern Extract session pool logic into ReadModifyWriteRowShimInner and make ReadModifyWriteRowShim<RowT> a stateless generic wrapper that pairs the inner with a RowAdapter passed from EnhancedBigtableStub. Change-Id: I56654ae6c4d0c051d01bfc85f672d474d192e2d8
mutianf
commented
Oct 8, 2026
- Remove stale TODO comments from TableAsync.readModifyWriteRow - Rephrase "must never be retried" to "should not be retried unless directed by server" for CheckAndMutateRow and ReadModifyWriteRow in TableBase - Extract shared buildRow logic into RowConverter utility, used by both ReadRowShim and ReadModifyWriteRowShim - Remove CheckAndMutateRowShim and ReadModifyWriteRowShimInner from ShimImpl until session routing is enabled; decorator methods now return the classic callable directly to avoid creating unused session pools Change-Id: I7a803404cbf5c945b7e03aac4c970f0ed31feec8
…M and RMW shims - Move buildRow logic from RowConverter into Util (no separate class needed) - Implement proper supports() in CheckAndMutateRowShim and ReadModifyWriteRowShimInner using session pool checks, matching ReadRowShimInner's pattern - Short-circuit routing in ShimImpl.decorateCheckAndMutateRow and decorateReadModifyWriteRow (return classic) until diversion by method is enabled; enabling routing is a one-line change per operation Change-Id: Iab114d444dc5dbf9645d46d96028056f759cb9ab
- Use inline named-arg comments for null descriptors in MaterializedViewAsync - Add explanation comment for RetryingVRpc with idempotent=false - Add circuit-breaker explanation to supports() in CheckAndMutateRowShim and ReadModifyWriteRowShimInner - Inline buildRow helper in ReadModifyWriteRowShim - Introduce WipFeatures constants and implement the disabled-shim guard pattern in ShimImpl
…wBuilderShim, move buildRowFromProto to RowAdapter - Move shouldRouteToSession circuit-breaker + MAX_CONSECUTIVE_UNIMPLEMENTED_FAILURES from ShimImpl to UnaryShim (same package as ops, no inverted dependency) - Add generic RowBuilderShim<ReqT, ProtoRespT, RowT> that wraps a raw-proto shim + RowAdapter + row-extractor lambda, eliminating per-op row-building boilerplate - Move buildRow helper from Util to RowAdapter.buildRowFromProto() default method - Rename ReadRowShimInner → ReadRowShim and ReadModifyWriteRowShimInner → ReadModifyWriteRowShim (old thin-factory wrappers removed) - Gate CheckAndMutateRow and ReadModifyWriteRow session paths behind WipFeatures flags
mutianf
force-pushed
the
readModifyWrite
branch
from
October 8, 2026 21:37
06686f4 to
ae23bb2
Compare
…uteToSession, and RowAdapter.buildRowFromProto
mutianf
commented
Oct 9, 2026
…dModifyWriteRowShim tests - Remove buildRowFromProto default method from the public RowAdapter interface to avoid silently affecting custom implementations; move the logic to a package-private static helper in RowBuilderShim where it is actually used. - Add ReadModifyWriteRowShimTest covering supports() (TableId/AuthorizedViewId, circuit-breaker threshold, unsupported TargetId) and call() happy paths. - Add TODO to CheckAndMutateRowShim to merge READ_WRITE pools with ReadModifyWriteRowShim once backend permission is confirmed. - Remove duplicate circuit-breaker comments from shim call sites; the explanation lives on UnaryShim.shouldRouteToSession().
… and ReadModifyWriteRow Extract ReadWriteSessionPools to hold the shared PERMISSION_READ_WRITE SessionPoolMaps. Both CheckAndMutateRowShim and ReadModifyWriteRowShim now use the same pool per table, halving session connections and unifying circuit-breaker failure tracking across the two RPCs.
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for the ReadModifyWriteRow operation in the session-based data path. Key changes include the addition of ReadModifyWriteRowShim and ReadWriteSessionPools to manage and share read-write session pools, the introduction of a generic RowBuilderShim to simplify row adaptation, and the integration of these components into TableAsync, AuthorizedViewAsync, and EnhancedBigtableStub. Additionally, work-in-progress feature flags have been added to control the rollout of these features. I have no further feedback to provide as there are no review comments.
…ndMutate and ReadModifyWriteRow" This reverts commit 4ca4370.
igorbernstein2
approved these changes
Oct 13, 2026
mutianf
enabled auto-merge (squash)
October 13, 2026 19:13
mutianf
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
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
3 participants
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.
Support SessionReadModifyWriteRow.
And some minor clean ups.