Repository navigation
feat: Session support for mutateRows - #14644
alexoneill wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for mutateRows (bulk mutations) across the Bigtable client's async API and compatibility layers, including TableAsync, AuthorizedViewAsync, and TableBase, along with a new MutateRowsShim implementation. The review feedback suggests importing com.google.common.base.Preconditions and adding a defensive null check for the client parameter in the MutateRowsShim constructor to ensure object integrity.
| import com.google.cloud.bigtable.data.v2.models.TargetId; | ||
| import io.grpc.Deadline; |
There was a problem hiding this comment.
Import com.google.common.base.Preconditions to support defensive null-checking of parameters in the constructor.
| import com.google.cloud.bigtable.data.v2.models.TargetId; | |
| import io.grpc.Deadline; | |
| import com.google.cloud.bigtable.data.v2.models.TargetId; | |
| import com.google.common.base.Preconditions; | |
| import io.grpc.Deadline; |
| public MutateRowsShim(Client client) { | ||
| tables = |
There was a problem hiding this comment.
Add a defensive null check for the client parameter in the constructor to ensure the object is initialized with a valid reference.
public MutateRowsShim(Client client) {
Preconditions.checkNotNull(client, "client is required");
tables =References
- Defensive null checks in constructors are appropriate to ensure object integrity, provided they are not redundant with upstream validation.
| return f; | ||
| } | ||
|
|
||
| // TODO: get deadline from compatibility layer |
There was a problem hiding this comment.
this comment can be removed. My PR is removing it in all other places.
There was a problem hiding this comment.
Done — removed the comment.
| return true; | ||
| } | ||
|
|
||
| public static boolean isIdempotent(SessionMutateRowsRequest req) { |
There was a problem hiding this comment.
Can we remove this method?
on line 192 in tableBase you can do something like:
ImmutableList<Mutation> mutations = request.getEntriesList().stream().flatMap(entry -> entry.getMutationsList().stream()).collect(toImmutableList());
Util.isIdempotent(mutations);
There was a problem hiding this comment.
Done — removed Util.isIdempotent(SessionMutateRowsRequest) and flattened the entry mutations in TableBase.mutateRows.
| private final SessionPoolMap<AuthorizedViewId, AuthorizedViewAsync> authViews; | ||
|
|
||
| public MutateRowsShim(Client client) { | ||
| tables = |
There was a problem hiding this comment.
we should consider share the pool with MutateRow.
See my PR (although is not approved yet 😂 )
https://github.com/googleapis/google-cloud-java/pull/14585/changes#diff-e47339d2678dd2991014d93d7eaa96d999bb53d535c753e67766f0582b2212f4
There was a problem hiding this comment.
Done — updated MutateRowsShim to share SessionPoolMap (SessionKey.Permission.WRITE) with MutateRowShim.
57573a1 to
85c58c9
Compare
85c58c9 to
e2111ee
Compare
mutianf
left a comment
mutianf
left a comment
There was a problem hiding this comment.
We're returning RateLimitInfo in SessionMutateRowsResponse, but nothing checks / uses it in the session client path?
| return sessionMap.apply( | ||
| (AuthorizedViewId) targetId, | ||
| SessionKey.Permission.WRITE, | ||
| v -> v.mutateRows(innerReq, deadline).thenApply(r -> null)); |
There was a problem hiding this comment.
Hmm, I think this only reports RPC level status? if there are entry level failures it'll get silently dropped?
| req.getEntriesList().stream() | ||
| .flatMap(entry -> entry.getMutationsList().stream()) | ||
| .collect(toImmutableList()); | ||
| boolean idempotent = Util.isIdempotent(mutations); |
There was a problem hiding this comment.
full copy of every mutation to compute one boolean
Util.isIdempotent short-circuits, but toImmutableList() materializes everything first — up to BulkMutation.MAX_MUTATION (100k) references per call on the hot path. Use flatMap(...).allMatch(...) or a nested loop with early return.
No description provided.