[Python BQ] Retry get_table for quota errors

[ahmedabu98]

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_filter to make a best-effort attempt at retrying only for transient reasons. Note that BigQuery also returns error code 403 for non-transient errors like accessDenied.

[ahmedabu98]

Run Python 3.8 Postcommit

[ahmedabu98]

Run Python 3.10 PostCommit

[Abacn]

LGTM

just a note - python postcommit currently failing bigquery read ITs: #28811

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (0b93830) 72.22% compared to head (bfdc47c) 72.27%.
Report is 791 commits behind head on master.

❗ Current head bfdc47c differs from pull request most recent head 4992a83. Consider uploading reports for the commit 4992a83 to get more accurate results

Files	Patch %	Lines
sdks/python/apache_beam/utils/retry.py	93.33%	1 Missing ⚠️

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              

Flag	Coverage Δ
python	82.83% <93.33%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kennknowles]

So obviously I have to ask - can you test it?

[liferoad]

You probably need to mock the call like https://github.com/apache/beam/pull/28475/files#diff-a7854eda9da1cdd78ece45e7529978d8296841b84626bee1f925483586b52b14

[ahmedabu98]

Yup, added some tests

[ahmedabu98]

Run Python 3.8 Postcommit

[ahmedabu98]

Run Python 3.10 Postcommit

[ahmedabu98]

Thanks for the review @satybald, PTAL

[ahmedabu98]

@satybald sorry for the delay, PTAL

[ahmedabu98]

@Abacn can you take another look at this?

[github-actions]

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @damccorm for label python.
R: @Abacn for label io.

Available commands:

• stop reviewer notifications - opt out of the automated review tooling
• remind me after tests pass - tag the comment author after tests pass
• waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

[ahmedabu98]

stop reviewer notifications

[Abacn]

Thanks for adding the test. Just have thing to confirm about the assumption over response

[Abacn]

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?

[ahmedabu98]

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'])  # >>> notFound

[Abacn]

Thanks for the detail. Anyways this is wrapped in a a fail-safe try clause so LGTM

[github-actions]

Stopping reviewer notifications for this pull request: requested by reviewer