[Python BQ] Retry get_table for quota errors #28820
Conversation
|
Run Python 3.8 Postcommit |
|
Run Python 3.10 PostCommit |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #28820 +/- ##
==========================================
+ Coverage 72.22% 72.27% +0.04%
==========================================
Files 684 684
Lines 101230 101248 +18
==========================================
+ Hits 73117 73173 +56
+ Misses 26534 26496 -38
Partials 1579 1579
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
So obviously I have to ask - can you test it? |
|
You probably need to mock the call like https://github.com/apache/beam/pull/28475/files#diff-a7854eda9da1cdd78ece45e7529978d8296841b84626bee1f925483586b52b14 |
|
Yup, added some tests |
|
Run Python 3.8 Postcommit |
|
Run Python 3.10 Postcommit |
|
Thanks for the review @satybald, PTAL |
|
@satybald sorry for the delay, PTAL |
|
@Abacn can you take another look at this? |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @damccorm for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
stop reviewer notifications |
| content = exception.content | ||
| if not isinstance(content, dict): | ||
| content = json.loads(exception.content) | ||
| return content["error"]["errors"][0]["reason"] in _RETRYABLE_REASONS |
There was a problem hiding this comment.
This is a deeply nested information. Is the response guaranteed (documented) to have this format? In any case, would you mind add a comment for the example error json loads, or is there an example error message could share?
There was a problem hiding this comment.
I'm basing this off of the structure of the exception returned from get_table():
bq = bigquery_tools.BigQueryWrapper()
try:
bq.get_table("google.com:clouddfe", "nonexistent", "nonexistent")
except Exception as e:
print(type(e)) # >>> <class 'apitools.base.py.exceptions.HttpNotFoundError'>
print(e.status_code) # >>> 404
print(type(e.content)) # >>> <class 'bytes'>
content = json.loads(e.content)
print(content.keys()) # >>> dict_keys(['error'])
print(content['error'].keys()) # >>> dict_keys(['code', 'message', 'errors', 'status', 'details'])
print(content['error']['errors'][0].keys()) # >>> dict_keys(['message', 'domain', 'reason', 'debugInfo'])
print(content['error']['errors'][0]['reason']) # >>> notFoundThere was a problem hiding this comment.
Thanks for the detail. Anyways this is wrapped in a a fail-safe try clause so LGTM
|
Stopping reviewer notifications for this pull request: requested by reviewer |
* retry get_table on quota errors * add tests * only retry on transient reasons
Python SDK's BigQuery get table method is only retried for server and timeout errors. We should extend this to also retry for quota errors (403:
quotaExceeded,rateLimitExceeded).Also adding some logic to
retry_on_server_errors_timeout_or_quota_issues_filterto make a best-effort attempt at retrying only for transient reasons. Note that BigQuery also returns error code 403 for non-transient errors likeaccessDenied.