feat(bigtable): Reroute Mutations Batcher to use data client - #18200
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
| ) | ||
| 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
There was a problem hiding this comment.
Would it be better if we wrap it in a standard grpc error (unknown or something) and wraps the cause, so we don't return FailedMutationEntryError to user directly?
There was a problem hiding this comment.
Sure, added. We do expect FailedMutationEntryError to always have a cause, but that feels like a worthwhile change either way
57774e9 to
87f74b5
Compare
| exc.index = None | ||
| exceptions.extend(exc_list) | ||
| except Exception as e: | ||
| except FailedMutationEntryError as e: |
There was a problem hiding this comment.
would task raise a FailedMutationEntry? Or it always return a list?
There was a problem hiding this comment.
It's expected to always return a list. The catch is only for handling unexpected failures in the batcher's background task
I think Kevin added this to avoid double-wrapping exceptions, but I think we can just keep the other except block here. We realistically don't expect to ever see FailedMutationEntry here, and the worst that would happen is a slightly uglier exception message
| # 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.
Would it be better if we wrap it in a standard grpc error (unknown or something) and wraps the cause, so we don't return FailedMutationEntryError to user directly?
a1ae8da to
29f69e6
Compare
29f69e6 to
3bbaad4
Compare
**Changes Made:** - Replaced mutations batcher implementation with one based off of the data client. - Reworked unit tests. - Added additional system tests.
3bbaad4 to
26f24d0
Compare
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