fix: honor custom retry in job.result() - #2302
Conversation
Sorry, something went wrong.
The `_AsyncJob.result()` method was not correctly passing the `retry` argument to the superclass's `result()` method when the `retry` object was the same as the default retry object. This caused the default retry settings to be ignored in some cases. This change modifies the `result()` method to always pass the `retry` argument to the superclass, ensuring that the provided retry settings are always honored. A new test case is added to verify that `job.result()` correctly handles both the default retry and a custom retry object.
| self._begin(retry=retry, timeout=timeout) | ||
|
|
||
| kwargs = {} if retry is DEFAULT_RETRY else {"retry": retry} | ||
| kwargs = {"retry": retry} |
There was a problem hiding this comment.
Do we need a kwargs dict here? Can't you just do return super(_AsyncJob, self).result(timeout=timeout, retry=retry)?
Sorry, something went wrong.
There was a problem hiding this comment.
Removed that line and reworked things as you suggest.
Sorry, something went wrong.
| assert result.output_rows == 1 | ||
|
|
||
| # We made all the calls we expected to. | ||
| assert conn.api_request.call_count == 3 |
There was a problem hiding this comment.
This test seems to ensure that the rpc is being retried, but not that it's using the retry configuration that was actually passed in.
If a regression made it fall back to DEFAULT_RETRY in the handwritten layer, or even the default retry configured in the gapic layer like before, wouldn't the tests still pass? Or is that tested elsewhere?
Sorry, something went wrong.
There was a problem hiding this comment.
Spent some time thinking about this PR.
Began to doubt whether checking the client._connection.api_request() (e.g. conn.api_request) method was the right way to go.
Turns out, the client._call_api() method wraps the call to client._connection.api_request(). The retry object is used by _call_api to manage any needed retries, but the retry object itself is NOT passed as a direct argument to client._connection.api_request. Nor is it expected to be.
Here's what we did:
- Mock client._call_api: we wrapped the original client._call_api using mock.patch.object with wraps=client._call_api. This lets the real _call_api (including the retry decorator) execute, but we can still inspect the calls.
- Inspect retry argument: we added a loop through wrapped_call_api.mock_calls to check the retry object passed as the first argument to _call_api.
- Conditional Assertions: Inside the loop, we added assertions to verify that the properties of the called_retry object match the expected_retry (which is either DEFAULT_RETRY or the "custom retry" object). we've included a specific check for the _timeout == 10.0 in the "custom retry" case.
- Kept Existing Assertions: The original assertions on job.state, result.output_rows, and conn.api_request.call_count are still in place.
Sorry, something went wrong.
PR created by the Librarian CLI to initialize a release. Merging this PR will auto trigger a release. Librarian Version: v0.7.0 Language Image: us-central1-docker.pkg.dev/cloud-sdk-librarian-prod/images-prod/python-librarian-generator@sha256:c8612d3fffb3f6a32353b2d1abd16b61e87811866f7ec9d65b59b02eb452a620 <details><summary>google-cloud-bigquery: 3.39.0</summary> ## [3.39.0](v3.38.0...v3.39.0) (2025-12-12) ### Features * adds support for Python runtime 3.14 (#2322) ([6065e14](6065e14c)) * Add ExternalRuntimeOptions to BigQuery routine (#2311) ([fa76e31](fa76e310)) ### Bug Fixes * remove ambiguous error codes from query retries (#2308) ([8bbd3d0](8bbd3d01)) * include `io.Base` in the `PathType` (#2323) ([b11e09c](b11e09cb)) * honor custom `retry` in `job.result()` (#2302) ([e118b02](e118b029)) ### Documentation * remove experimental annotations from GA features (#2303) ([1f1f9d4](1f1f9d41)) </details> Co-authored-by: Daniel Sanche <d.sanche14@gmail.com>
The _AsyncJob.result() method was not correctly passing the right arguments to the superclass's result() method when the retry object was expected to be the same as the DEFAULT_RETRY object. Passing an empty dict was a way of simply letting the google.api_core handle things.
{} if retry is DEFAULT_RETRY else {"retry": retry}When the retry object is instantiated and when the DEFAULT_RETRY object is instantiated, these two elements are separate objects. This means that because the is keyword checks for identity NOT equality that code path never evaluates to True and is not used as one would expect.
This leads to subtle bugs that this PR corrects by simply passing the retry object and relying on the google.api_core to understand what a DEFAULT_RETRY value is and managing it appropriately.
A new test case is added to verify that job.result() correctly handles both the default retry use case and a custom retry object use case.
Fixes #2210 🦕