feat(bigtable): Reroute Mutations Batcher to use data client - #18200
daniel-sanche wants to merge 9 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 refactors the MutationsBatcher to delegate batching, queueing, and flow control to the underlying table implementation, removing redundant internal classes. It also updates the exception handling across both sync and async batchers to ensure that MutationsExceptionGroup only contains FailedMutationEntryError instances. A review comment points out a potential issue where unpacking error.__cause__ could result in None being added to the exceptions queue, and suggests a defensive fallback to the error itself.
dbfc051 to
57774e9
Compare
| exceptions.extend( | ||
| [ | ||
| FailedMutationEntryError( | ||
| failed_idx=None, failed_mutation_entry=entry, cause=e |
There was a problem hiding this comment.
what does field_index=None mean? should it be the index of the current entry?
There was a problem hiding this comment.
index doesn't make as much sense for the user in the batcher context, because the user adds mutations one at a time, and has no context into what the underlying batch looks like. The batcher strips out the indexes for this reason.
| ) | ||
| return status_pb2.Status( | ||
| code=code_pb2.Code.UNKNOWN, | ||
| message="An unknown error has occurred", |
There was a problem hiding this comment.
maybe change this to GoogleApiCAllError with Unknown status code
| # FailedMutationEntryError always has an Exception cause; | ||
| # defensively fall back to error itself if __cause__ is None. | ||
| cause = error.__cause__ if error.__cause__ is not None else error | ||
| self._exceptions.put(cause) |
There was a problem hiding this comment.
could there be other types of error? we should put them in the exception list
There was a problem hiding this comment.
We don't expect there to be, but we can add a guard to be safe
|
|
||
| return responses | ||
| self._close_batcher() | ||
| self._init_batcher() |
There was a problem hiding this comment.
is it expensive to keep close and reopen batcher? would it be better if the data client batcher expose a flush method?
There was a problem hiding this comment.
I believe the original plan was to implement flush, but Igor and Kevin had a discussion, and decided to just do this instead. Maybe we can find a record of the decision
I remember when we discussed adding flush in the past, there were quite a few edge cases to consider
| assert len(all_results) == num_sent | ||
|
|
||
|
|
||
| def test_mutations_batcher_exceptions(data_table, rows_to_delete): |
There was a problem hiding this comment.
can we add a test for RPC level error?
**Changes Made:** - Replaced mutations batcher implementation with one based off of the data client. - Reworked unit tests. - Added additional system tests.
57774e9 to
87f74b5
Compare
This branch has not been deployed
3 participants
Migrating over @gkevinzheng PR from bigtable monorepo googleapis/python-bigtable#1309
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