-
Notifications
You must be signed in to change notification settings - Fork 324
fix: honor custom retry in job.result()
#2302
Changes from 1 commit
79c44bc
d9cc9f8
5476f02
71e31b3
0114337
028f741
e0b9ec7
0148336
5b6012a
e8d5fb0
a3fbcf1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -615,3 +615,45 @@ def test_query_and_wait_retries_job_for_DDL_queries(global_time_lock): | |
| _, kwargs = calls[3] | ||
| assert kwargs["method"] == "POST" | ||
| assert kwargs["path"] == query_request_path | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "result_retry", | ||
| [ | ||
| pytest.param( | ||
| {}, | ||
| id="default retry", | ||
|
chalmerlowe marked this conversation as resolved.
Outdated
|
||
| ), | ||
| pytest.param( | ||
| {"retry": google.cloud.bigquery.retry.DEFAULT_RETRY.with_timeout(timeout=10.0)}, | ||
| id="custom retry object", | ||
|
chalmerlowe marked this conversation as resolved.
Outdated
|
||
| ), | ||
| ], | ||
| ) | ||
| def test_retry_load_job_result(result_retry, PROJECT, DS_ID): | ||
| from google.cloud.bigquery.dataset import DatasetReference | ||
| from google.cloud.bigquery.job.load import LoadJob | ||
|
|
||
| client = make_client() | ||
| conn = client._connection = make_connection( | ||
| dict( | ||
| status=dict(state="RUNNING"), | ||
| jobReference={"jobId": "id_1"}, | ||
| ), | ||
| google.api_core.exceptions.ServiceUnavailable("retry me"), | ||
| dict( | ||
| status=dict(state="DONE"), | ||
| jobReference={"jobId": "id_1"}, | ||
| statistics={"load": {"outputRows": 1}}, | ||
| ), | ||
| ) | ||
|
|
||
| table_ref = DatasetReference(project=PROJECT, dataset_id=DS_ID).table("new_table") | ||
| job = LoadJob("id_1", source_uris=None, destination=table_ref, client=client) | ||
| result = job.result(**result_retry) | ||
|
|
||
| assert job.state == "DONE" | ||
| assert result.output_rows == 1 | ||
|
|
||
| # We made all the calls we expected to. | ||
| assert conn.api_request.call_count == 3 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spent some time thinking about this PR. Began to doubt whether checking the Turns out, the Here's what we did:
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a kwargs dict here? Can't you just do
return super(_AsyncJob, self).result(timeout=timeout, retry=retry)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed that line and reworked things as you suggest.