tests(bigtable): System/compatibility tests for testing shim compatiblity - #17901
tests(bigtable): System/compatibility tests for testing shim compatiblity#17901daniel-sanche wants to merge 5 commits into
Conversation
Additional system tests for testing shim compatibility with the classic client interface. Also fixes a bug where encountering a retriable error after another retriable error mid-stream raised an exception instead of retrying. Fixes #1156 --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
![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 updates the Bigtable row data handler to propagate the retry configuration when re-establishing a read stream on error. It also introduces comprehensive system tests to validate retry behavior, timeouts, and input error handling. The review feedback correctly identifies a missing COL_NAME2 variable definition in the test file that would lead to a NameError, and offers a memory-efficient optimization using itertools to avoid allocating a massive list for mock side effects.
| CELL_VAL_TRUE = b"true" | ||
| CELL_VAL_FALSE = b"false" | ||
| INT_COL_NAME = 67890 |
There was a problem hiding this comment.
The variable COL_NAME2 is used in test_table_append_row_input_errors (on lines 1194 and 1201) but is not defined anywhere in the module. This will cause a NameError when the tests are executed. Defining COL_NAME2 at the module level resolves this issue.
| CELL_VAL_TRUE = b"true" | |
| CELL_VAL_FALSE = b"false" | |
| INT_COL_NAME = 67890 | |
| CELL_VAL_TRUE = b"true" | |
| CELL_VAL_FALSE = b"false" | |
| COL_NAME2 = b"col-name2" | |
| INT_COL_NAME = 67890 |
There was a problem hiding this comment.
COL_NAME_2 is defined here, added before the change
| mutate_mock.side_effect = [initial_error_response] + [ | ||
| followup_error_response | ||
| ] * 1000000 |
There was a problem hiding this comment.
Creating a list of 1,000,000 elements using [followup_error_response] * 1000000 is memory-inefficient and unnecessary. Using itertools.chain and itertools.repeat achieves the same result with O(1) memory complexity. Ensure itertools is imported at the module level rather than inside the function.
| mutate_mock.side_effect = [initial_error_response] + [ | |
| followup_error_response | |
| ] * 1000000 | |
| mutate_mock.side_effect = itertools.chain( | |
| [initial_error_response], itertools.repeat(followup_error_response) | |
| ) |
References
- Do not import modules or classes inside functions if they are already imported at the module level.
|
Please could you resolve the failing presubmits and mark this PR ready for review? |
|
|
||
| self._row_merger = _RowMerger(self._row_merger.last_seen_row_key) | ||
| self.response_iterator = self.read_method(retry_request) | ||
| self.response_iterator = self.read_method(retry_request, retry=self.retry) |
There was a problem hiding this comment.
What's in self.retry? do we need to update attempt number or backoff time in it?
There was a problem hiding this comment.
It's referring to a Retry object from api_core. This was Kevin's explaination of the change:
This change was needed because the read_rows method that gets passed in gets called with the GAPIC default retry instead of the retry that the user passed in or DEFAULT_READ_RETRY for this instance. If there's an error with this specific instance of self.read_method it doesn't get retried and errors out.
For example, if a retriable InternalError occurred mid-stream, then in this on_error handler the same error occurred, the error in the on_error handler would not be retried because the specific retry was not passed in for that RPC call.
So this was an attempt to fix a bug he ran into, where customized retry objects would not be propagated between attempts
| CELL_VAL_TRUE = b"true" | ||
| CELL_VAL_FALSE = b"false" | ||
| INT_COL_NAME = 67890 |
4 participants
Migrating over @gkevinzheng PR from bigtable monorepo googleapis/python-bigtable#1218
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