feat(bigtable): Rerouted CheckAndMutateRows and ReadModifyWriteRows to data client - #18190
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 Bigtable row and row filter implementations to delegate to the new data client (google.cloud.bigtable.data) instead of constructing legacy v2 protobufs directly, while maintaining backwards compatibility. The review feedback points out a critical runtime AttributeError in the mock _Table class within the unit tests, and suggests adding a descriptive error message to the AttributeError raised by _MappableAttributesMixin.__getattr__ to improve debuggability.
ad30839 to
c1a1d03
Compare
|
We can ignore the import profiler checks here. It is expected that imports will be a bit slower at first, since we are pulling in the data client (over time, we will be removing legacy client classes to make up for it) |
daniel-sanche
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
**Changes made:** - `Row` objects hold `Mutation` and `ReadModifyWriteRowRule` objects from the data client rather than raw protos. - Rerouted `ConditionalRow.commit` and `AppendRow.commit` (CheckAndMutateRows and ReadModifyWriteRows respectively) to use the data client, or more specifically, `self._table._table_impl` - Added function `DirectRow._to_mutation_pbs` for retrieving mutations in proto form for the current `MutateRows` implementation, as well as for `DirectRow.get_mutations_size`. - Removed unnecessary helper functions and tests for helper functions
473534e to
68f942b
Compare
| # Use -1 for current Bigtable server time. | ||
| timestamp_micros = -1 | ||
| # Use current Bigtable server time. | ||
| timestamp_micros = mutations._SERVER_SIDE_TIMESTAMP |
There was a problem hiding this comment.
wow, this is very dangerous, but I guess this is preserving old behaviors? does this get retried?
There was a problem hiding this comment.
Yes, this is the old behaviour.
The new data client will see this as a non-idempotent, and not retry. The old implementation would retry mutations (but not check_and_mutate)
Is that what you'd expect?
4 participants
Migrating over @gkevinzheng PR from bigtable monorepo googleapis/python-bigtable#1257
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