-
Notifications
You must be signed in to change notification settings - Fork 309
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.
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
fix: retry 'job exceeded rate limits' for DDL queries #1794
Conversation
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.
Thanks!
I have some feedback regarding testing. I think it may make more sense to follow the pattern I use here, instead:
python-bigquery/tests/unit/test__job_helpers.py
Lines 388 to 447 in 1f96439
def test_query_and_wait_retries_job(): | |
freezegun.freeze_time(auto_tick_seconds=100) | |
client = mock.create_autospec(Client) | |
client._call_api.__name__ = "_call_api" | |
client._call_api.__qualname__ = "Client._call_api" | |
client._call_api.__annotations__ = {} | |
client._call_api.__type_params__ = () | |
client._call_api.side_effect = ( | |
google.api_core.exceptions.BadGateway("retry me"), | |
google.api_core.exceptions.InternalServerError("job_retry me"), | |
google.api_core.exceptions.BadGateway("retry me"), | |
{ | |
"jobReference": { | |
"projectId": "response-project", | |
"jobId": "abc", | |
"location": "response-location", | |
}, | |
"jobComplete": True, | |
"schema": { | |
"fields": [ | |
{"name": "full_name", "type": "STRING", "mode": "REQUIRED"}, | |
{"name": "age", "type": "INT64", "mode": "NULLABLE"}, | |
], | |
}, | |
"rows": [ | |
{"f": [{"v": "Whillma Phlyntstone"}, {"v": "27"}]}, | |
{"f": [{"v": "Bhetty Rhubble"}, {"v": "28"}]}, | |
{"f": [{"v": "Phred Phlyntstone"}, {"v": "32"}]}, | |
{"f": [{"v": "Bharney Rhubble"}, {"v": "33"}]}, | |
], | |
}, | |
) | |
rows = _job_helpers.query_and_wait( | |
client, | |
query="SELECT 1", | |
location="request-location", | |
project="request-project", | |
job_config=None, | |
page_size=None, | |
max_results=None, | |
retry=retries.Retry( | |
lambda exc: isinstance(exc, google.api_core.exceptions.BadGateway), | |
multiplier=1.0, | |
).with_deadline( | |
200.0 | |
), # Since auto_tick_seconds is 100, we should get at least 1 retry. | |
job_retry=retries.Retry( | |
lambda exc: isinstance(exc, google.api_core.exceptions.InternalServerError), | |
multiplier=1.0, | |
).with_deadline(600.0), | |
) | |
assert len(list(rows)) == 4 | |
# For this code path, where the query has finished immediately, we should | |
# only be calling the jobs.query API and no other request path. | |
request_path = "/projects/request-project/queries" | |
for call in client._call_api.call_args_list: | |
_, kwargs = call | |
assert kwargs["method"] == "POST" | |
assert kwargs["path"] == request_path |
It may make sense to also add a test for just the predicate itself:
python-bigquery/tests/unit/test_retry.py
Line 109 in 1f96439
def test_DEFAULT_JOB_RETRY_predicate(): |
2db8bf4
to
ef36bf2
Compare
ce34c69
to
fd28904
Compare
Co-authored-by: Tim Swast <swast@google.com>
89a1bd3
to
ffd6e38
Compare
tests/unit/test_job_retry.py
Outdated
|
||
_, kwargs = calls[2] | ||
assert kwargs["method"] == "POST" | ||
assert kwargs["path"].startswith(response_path) |
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.
This is an interesting one. Not sure why we'd be doing a POST
to this path. That'd indicate that we are doing a jobs insert call, which really shouldn't be happening.
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.
Added a todo statement for the fix in #1797
46f43a5
to
0d0e2f5
Compare
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.
Thanks!
Thanks @kiraksi! |
@@ -22,6 +22,10 @@ | |||
import google.api_core.retry | |||
import freezegun | |||
|
|||
from google.cloud.bigquery.client import Client | |||
from google.cloud.bigquery import _job_helpers | |||
from google.cloud.bigquery.retry import DEFAULT_JOB_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.
Import modules not classes / methods.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #1790 🦕