Skip to content

tests: System/compatibility tests for testing shim compatiblity #1218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: v3_staging
Choose a base branch
from
+834 −4
Open

tests: System/compatibility tests for testing shim compatiblity #1218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
807ae39
tests: System/compatibility tests for testing shim compatiblity
gkevinzheng Sep 30, 2025
e86899a
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Oct 14, 2025
0426ce2
Use 3.12 instead of 3.8 for system tests
gkevinzheng Oct 14, 2025
cca6ccd
linting
gkevinzheng Oct 14, 2025
1bd0230
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Oct 14, 2025
b3ce1cd
Skipped sample_row_keys test on emulator
gkevinzheng Oct 15, 2025
e06550e
Merge branch 'v3_staging' into system-tests-legacy
gkevinzheng Oct 20, 2025
4499e4b
Use 3.12 instead of 3.8 for system tests
gkevinzheng Oct 14, 2025
a895bc2
Addressed review feedback
gkevinzheng Oct 21, 2025
a16360c
Update test_data_api.py
gkevinzheng Oct 21, 2025
533dba1
Delete test.out
gkevinzheng Oct 21, 2025
68a91be
Delete .kokoro/presubmit/system-3.12.cfg
gkevinzheng Oct 21, 2025
72cc5bb
Delete bigtable_delete.sh
gkevinzheng Oct 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion google/cloud/bigtable/row_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ def _on_error(self, exc):
retry_request = self._create_retry_request()

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this change needed?

Copy link
Contributor Author

@gkevinzheng gkevinzheng Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit torn, because this does feel like a very real bug. But the code here is also very complex, so I'm hesitant to make changes to the retry policy in case there are side-effects we're not considering

But I guess this is coming in the context of adding more system tests. Do you think the tests we have now are sufficient to catch any errors?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, on second thought, this is a PR into the v3_staging branch, not main. We will be doing much more extensive changes here as part of the shim, before cutting a release. So I think adding this fix makes sense

def _read_next(self):
"""Helper for :meth:`__iter__`."""
Expand Down
Loading
Loading