chore(bigtable): Added an internal batch completed callback to the data client mutations batcher - #18199
chore(bigtable): Added an internal batch completed callback to the data client mutations batcher#18199daniel-sanche wants to merge 4 commits 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 a user batch completed callback to both the async and sync auto-generated MutationsBatcher classes, and refactors status-extraction logic into a shared helper function _get_statuses_from_mutations_exception_group in _helpers.py. The review feedback highlights critical issues with using list multiplication (e.g., [status_pb2.Status(...)] * size) which creates multiple references to the same mutable Status object, recommending list comprehensions instead. Additionally, the reviewer advises wrapping the user callback execution in a try-except block within the finally clause to prevent callback exceptions from masking other active exceptions, and suggests using module-level protobuf constants for consistency.
…atcher (#1308) **Changes made:** - Refactored logic from `Table.mutate_rows` from producing a list of `Status` protos from a `MutationsExceptionGroup` - Added private keyword argument for a batch completion callback in the MutationsBatcher. - Added unit tests/system tests.
504536b to
b7ec06b
Compare
| subexc.index = None | ||
| return list(e.exceptions) | ||
| else: | ||
| statuses = [status_pb2.Status(code=code_pb2.OK) for _ in range(len(batch))] |
There was a problem hiding this comment.
Is it guaranteed that when we land in this else block, there won't be any rpc level or entry level failures?
| # 3. In the case of a RetryExceptionGroup, we use terminal exception in the exception | ||
| # group and process that. | ||
| statuses = [status_pb2.Status(code=code_pb2.OK) for _ in range(batch_size)] | ||
| for error in exc_group.exceptions: |
There was a problem hiding this comment.
what handles the rpc level error?
3 participants
Migrating over @gkevinzheng PR from bigtable monorepo googleapis/python-bigtable#1308
Original description:
Additional Changes:
Note to reviewers: This PR has already been reviewed and merged to a staging branch, with the intention of doing a single merge to main. We are now planning to slowly rollout these changes back to the main branch. Minimal re-review should be necessary