feat(bigtable): Rerouted MutateRows to use the data client - #18195
feat(bigtable): Rerouted MutateRows to use the data client#18195daniel-sanche wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the mutate_rows method in google/cloud/bigtable/table.py to delegate bulk mutation execution directly to the underlying data client implementation (self._table_impl.bulk_mutate_rows), allowing the removal of the legacy _RetryableMutateRowsWorker class and its associated helper functions. The review feedback correctly identifies a critical bug where an AttributeError will occur if retry is None, and points out the loss of defensive input validation checks for row types and table mismatches that were previously present.
734f5f0 to
fbf671c
Compare
| else: | ||
| operation_timeout = retry.deadline | ||
|
|
||
| attempt_timeout = timeout |
There was a problem hiding this comment.
could someone configure timeout but not retry deadline and have timeout > default retry deadline?
There was a problem hiding this comment.
The data client handles this by setting the attempt_timeout = min(operation_timeout, attempt_timeout)
| ] # By default, return status OKs for everything | ||
|
|
||
| try: | ||
| self._table_impl.bulk_mutate_rows( |
There was a problem hiding this comment.
this already wraps the retry layer, correct?
There was a problem hiding this comment.
I'm not sure I understand the question, but bulk_mutate_rows handles retries internally. The retry object passed in is just used to find deadlines and other arguments to pass through
Maybe we should raise an exception if the passed-in retry has fields set we don't support, like on_error? I think that was a potential breaking change Kevin raised. There's some functionality that isn't exposed, but may have been previously
There was a problem hiding this comment.
Yeah, I just wanted to make sure the data client handles retries
afa1c86 to
c26c9ec
Compare
c26c9ec to
bba5a3e
Compare
| ] # By default, return status OKs for everything | ||
|
|
||
| try: | ||
| self._table_impl.bulk_mutate_rows( |
There was a problem hiding this comment.
Yeah, I just wanted to make sure the data client handles retries
Migrating over @gkevinzheng PR from bigtable monorepo googleapis/python-bigtable#1290
Original description:
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