You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "ahmedabu98 (via GitHub)" <gi...@apache.org> on 2023/10/04 13:11:28 UTC
[PR] [Python BQ] Retry get_table for quota errors [beam]
ahmedabu98 opened a new pull request, #28820:
URL: https://github.com/apache/beam/pull/28820
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`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1747609763
You probably need to mock the call like https://github.com/apache/beam/pull/28475/files#diff-a7854eda9da1cdd78ece45e7529978d8296841b84626bee1f925483586b52b14
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "satybald (via GitHub)" <gi...@apache.org>.
satybald commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1357725890
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -166,17 +168,36 @@ def retry_on_server_errors_and_timeout_filter(exception):
def retry_on_server_errors_timeout_or_quota_issues_filter(exception):
- """Retry on server, timeout and 403 errors.
+ """Retry on server, timeout, 429, and some 403 errors.
- 403 errors can be accessDenied, billingNotEnabled, and also quotaExceeded,
- rateLimitExceeded."""
+ 403 errors from BigQuery include both non-transient (accessDenied,
+ billingNotEnabled) and transient errors (rateLimitExceeded).
+ Only retry transient errors."""
if HttpError is not None and isinstance(exception, HttpError):
- if exception.status_code == 403:
+ if exception.status_code == 429:
return True
+ if exception.status_code == 403:
+ try:
+ # attempt to extract the reason and check if it's retryable
+ return exception.content["error"]["errors"][0][
+ "reason"] in _RETRYABLE_REASONS
+ except (KeyError, IndexError, TypeError):
+ _LOGGER.debug(
Review Comment:
can we make it info or warning? looks to me an important message to be aware
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1881758389
stop reviewer notifications
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1445279611
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -166,17 +168,38 @@ def retry_on_server_errors_and_timeout_filter(exception):
def retry_on_server_errors_timeout_or_quota_issues_filter(exception):
- """Retry on server, timeout and 403 errors.
+ """Retry on server, timeout, 429, and some 403 errors.
- 403 errors can be accessDenied, billingNotEnabled, and also quotaExceeded,
- rateLimitExceeded."""
+ 403 errors from BigQuery include both non-transient (accessDenied,
+ billingNotEnabled) and transient errors (rateLimitExceeded).
+ Only retry transient errors."""
if HttpError is not None and isinstance(exception, HttpError):
- if exception.status_code == 403:
+ if exception.status_code == 429:
return True
+ if exception.status_code == 403:
+ try:
+ # attempt to extract the reason and check if it's retryable
+ content = exception.content
+ if not isinstance(content, dict):
+ content = json.loads(exception.content)
+ return content["error"]["errors"][0]["reason"] in _RETRYABLE_REASONS
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1347756779
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -57,6 +60,18 @@
# pylint: enable=wrong-import-order, wrong-import-position
_LOGGER = logging.getLogger(__name__)
+_RETRYABLE_REASONS = [
+ "rateLimitExceeded", "quotaExceeded", "internalError", "backendError"
+]
+_RETRYABLE_TYPES = (
Review Comment:
Actually, these aren't needed. There's logic elsewhere in this file that takes care of them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1881737781
@Abacn can you take another look at this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1746855915
Run Python 3.8 Postcommit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1746930711
So obviously I have to ask - can you test it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1748976934
Run Python 3.8 Postcommit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1347769950
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -42,9 +43,11 @@
try:
from apitools.base.py.exceptions import HttpError
from google.api_core.exceptions import GoogleAPICallError
+ from google.api_core import exceptions
Review Comment:
Removed as this is no longer needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1748977306
Run Python 3.10 Postcommit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1747640784
Yup, added some tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1881759843
Stopping reviewer notifications for this pull request: requested by reviewer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1417863371
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -166,17 +168,36 @@ def retry_on_server_errors_and_timeout_filter(exception):
def retry_on_server_errors_timeout_or_quota_issues_filter(exception):
- """Retry on server, timeout and 403 errors.
+ """Retry on server, timeout, 429, and some 403 errors.
- 403 errors can be accessDenied, billingNotEnabled, and also quotaExceeded,
- rateLimitExceeded."""
+ 403 errors from BigQuery include both non-transient (accessDenied,
+ billingNotEnabled) and transient errors (rateLimitExceeded).
+ Only retry transient errors."""
if HttpError is not None and isinstance(exception, HttpError):
- if exception.status_code == 403:
+ if exception.status_code == 429:
return True
+ if exception.status_code == 403:
+ try:
+ # attempt to extract the reason and check if it's retryable
+ return exception.content["error"]["errors"][0][
Review Comment:
Couldn't find a way to reproduce a quota error but I was able to look at the HttpError structure with this:
```python
try:
table = 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
```
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -166,17 +168,36 @@ def retry_on_server_errors_and_timeout_filter(exception):
def retry_on_server_errors_timeout_or_quota_issues_filter(exception):
- """Retry on server, timeout and 403 errors.
+ """Retry on server, timeout, 429, and some 403 errors.
- 403 errors can be accessDenied, billingNotEnabled, and also quotaExceeded,
- rateLimitExceeded."""
+ 403 errors from BigQuery include both non-transient (accessDenied,
+ billingNotEnabled) and transient errors (rateLimitExceeded).
+ Only retry transient errors."""
if HttpError is not None and isinstance(exception, HttpError):
- if exception.status_code == 403:
+ if exception.status_code == 429:
return True
+ if exception.status_code == 403:
+ try:
+ # attempt to extract the reason and check if it's retryable
+ return exception.content["error"]["errors"][0][
+ "reason"] in _RETRYABLE_REASONS
+ except (KeyError, IndexError, TypeError):
+ _LOGGER.debug(
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1749366938
Thanks for the review @satybald, PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "satybald (via GitHub)" <gi...@apache.org>.
satybald commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1357702917
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -166,17 +168,36 @@ def retry_on_server_errors_and_timeout_filter(exception):
def retry_on_server_errors_timeout_or_quota_issues_filter(exception):
- """Retry on server, timeout and 403 errors.
+ """Retry on server, timeout, 429, and some 403 errors.
- 403 errors can be accessDenied, billingNotEnabled, and also quotaExceeded,
- rateLimitExceeded."""
+ 403 errors from BigQuery include both non-transient (accessDenied,
+ billingNotEnabled) and transient errors (rateLimitExceeded).
+ Only retry transient errors."""
if HttpError is not None and isinstance(exception, HttpError):
- if exception.status_code == 403:
+ if exception.status_code == 429:
return True
+ if exception.status_code == 403:
+ try:
+ # attempt to extract the reason and check if it's retryable
+ return exception.content["error"]["errors"][0][
Review Comment:
could we run a pipeline to double check that his get parsed correctly? Might enougth to trigger 403- foridden exception to check if this retry works or not
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1347747794
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -168,15 +183,29 @@ def retry_on_server_errors_and_timeout_filter(exception):
def retry_on_server_errors_timeout_or_quota_issues_filter(exception):
"""Retry on server, timeout and 403 errors.
- 403 errors can be accessDenied, billingNotEnabled, and also quotaExceeded,
- rateLimitExceeded."""
+ 403 errors include both transient (accessDenied, billingNotEnabled) and
+ non-transient errors (quotaExceeded, rateLimitExceeded). Only retry transient
+ errors."""
if HttpError is not None and isinstance(exception, HttpError):
if exception.status_code == 403:
+ try:
+ # attempt to extract the reason and check if it's retryable
+ return exception.content["error"]["errors"][0][
+ "reason"] in _RETRYABLE_REASONS
+ except Exception:
Review Comment:
We can get `KeyError, IndexError, TypeError` from this line. Sure I'll list them out instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1881745939
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1445298920
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -166,17 +168,38 @@ def retry_on_server_errors_and_timeout_filter(exception):
def retry_on_server_errors_timeout_or_quota_issues_filter(exception):
- """Retry on server, timeout and 403 errors.
+ """Retry on server, timeout, 429, and some 403 errors.
- 403 errors can be accessDenied, billingNotEnabled, and also quotaExceeded,
- rateLimitExceeded."""
+ 403 errors from BigQuery include both non-transient (accessDenied,
+ billingNotEnabled) and transient errors (rateLimitExceeded).
+ Only retry transient errors."""
if HttpError is not None and isinstance(exception, HttpError):
- if exception.status_code == 403:
+ if exception.status_code == 429:
return True
+ if exception.status_code == 403:
+ try:
+ # attempt to extract the reason and check if it's retryable
+ content = exception.content
+ if not isinstance(content, dict):
+ content = json.loads(exception.content)
+ return content["error"]["errors"][0]["reason"] in _RETRYABLE_REASONS
Review Comment:
I'm basing this off of the structure of the exception returned from `get_table()`:
```python
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "satybald (via GitHub)" <gi...@apache.org>.
satybald commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1347688387
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -42,9 +43,11 @@
try:
from apitools.base.py.exceptions import HttpError
from google.api_core.exceptions import GoogleAPICallError
+ from google.api_core import exceptions
Review Comment:
to make code more readable, would it be better to use import alias to clearly distinguish between `import requests.exceptions`
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -168,15 +183,29 @@ def retry_on_server_errors_and_timeout_filter(exception):
def retry_on_server_errors_timeout_or_quota_issues_filter(exception):
"""Retry on server, timeout and 403 errors.
- 403 errors can be accessDenied, billingNotEnabled, and also quotaExceeded,
- rateLimitExceeded."""
+ 403 errors include both transient (accessDenied, billingNotEnabled) and
+ non-transient errors (quotaExceeded, rateLimitExceeded). Only retry transient
+ errors."""
if HttpError is not None and isinstance(exception, HttpError):
if exception.status_code == 403:
+ try:
+ # attempt to extract the reason and check if it's retryable
+ return exception.content["error"]["errors"][0][
+ "reason"] in _RETRYABLE_REASONS
+ except Exception:
+ _LOGGER.debug(
+ "Could not determine if HttpError is non-transient. Will retry: %s",
+ exception)
return True
Review Comment:
we shouldn't blindly retry on all `403` exceptions. Forbidden exceptions belongs to permanent exceptions.
Here's a guide from GCP about healthy retry policies:
https://cloud.google.com/storage/docs/retry-strategy
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -57,6 +60,18 @@
# pylint: enable=wrong-import-order, wrong-import-position
_LOGGER = logging.getLogger(__name__)
+_RETRYABLE_REASONS = [
+ "rateLimitExceeded", "quotaExceeded", "internalError", "backendError"
Review Comment:
why do we need to retry on `quotaExceeded`? Once the quota exceeded it needs a manual intervention or back-off of substantial amount of time.
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -168,15 +183,29 @@ def retry_on_server_errors_and_timeout_filter(exception):
def retry_on_server_errors_timeout_or_quota_issues_filter(exception):
"""Retry on server, timeout and 403 errors.
- 403 errors can be accessDenied, billingNotEnabled, and also quotaExceeded,
- rateLimitExceeded."""
+ 403 errors include both transient (accessDenied, billingNotEnabled) and
+ non-transient errors (quotaExceeded, rateLimitExceeded). Only retry transient
+ errors."""
if HttpError is not None and isinstance(exception, HttpError):
if exception.status_code == 403:
+ try:
+ # attempt to extract the reason and check if it's retryable
+ return exception.content["error"]["errors"][0][
+ "reason"] in _RETRYABLE_REASONS
+ except Exception:
Review Comment:
Instead of generic `Exception` shoud it be `KeyError`?
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -57,6 +60,18 @@
# pylint: enable=wrong-import-order, wrong-import-position
_LOGGER = logging.getLogger(__name__)
+_RETRYABLE_REASONS = [
+ "rateLimitExceeded", "quotaExceeded", "internalError", "backendError"
+]
+_RETRYABLE_TYPES = (
Review Comment:
Should we be DRY here and avoid `if exceptions`? i.e.
```
_RETRYABLE_EXCEPTIONS_FROM_MODULE = [
exceptions.TooManyRequests,
exceptions.InternalServerError,
exceptions.BadGateway,
exceptions.ServiceUnavailable,
exceptions.DeadlineExceeded
] if exceptions else []
_RETRYABLE_TYPES = tuple([
*_RETRYABLE_EXCEPTIONS_FROM_MODULE,
requests.exceptions.ConnectionError,
requests.exceptions.Timeout,
ConnectionError
])
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1347716789
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -57,6 +60,18 @@
# pylint: enable=wrong-import-order, wrong-import-position
_LOGGER = logging.getLogger(__name__)
+_RETRYABLE_REASONS = [
+ "rateLimitExceeded", "quotaExceeded", "internalError", "backendError"
Review Comment:
For some cases, the quota exceeded error could be transient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1843532306
@satybald sorry for the delay, PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1347747381
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -168,15 +183,29 @@ def retry_on_server_errors_and_timeout_filter(exception):
def retry_on_server_errors_timeout_or_quota_issues_filter(exception):
"""Retry on server, timeout and 403 errors.
- 403 errors can be accessDenied, billingNotEnabled, and also quotaExceeded,
- rateLimitExceeded."""
+ 403 errors include both transient (accessDenied, billingNotEnabled) and
+ non-transient errors (quotaExceeded, rateLimitExceeded). Only retry transient
+ errors."""
if HttpError is not None and isinstance(exception, HttpError):
if exception.status_code == 403:
+ try:
+ # attempt to extract the reason and check if it's retryable
+ return exception.content["error"]["errors"][0][
+ "reason"] in _RETRYABLE_REASONS
+ except Exception:
+ _LOGGER.debug(
+ "Could not determine if HttpError is non-transient. Will retry: %s",
+ exception)
return True
Review Comment:
Thanks for this link, I added some of these status codes as retryable.
Looks like another odd behavior is BigQuery can return [`notImplemented` as 501](https://cloud.google.com/bigquery/docs/error-messages#:~:text=notImplemented,for%20more%20information.)
> we shouldn't blindly retry on all 403 exceptions. Forbidden exceptions belongs to permanent exceptions.
SGTM
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -57,6 +60,18 @@
# pylint: enable=wrong-import-order, wrong-import-position
_LOGGER = logging.getLogger(__name__)
+_RETRYABLE_REASONS = [
+ "rateLimitExceeded", "quotaExceeded", "internalError", "backendError"
Review Comment:
Removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1347746691
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -57,6 +60,18 @@
# pylint: enable=wrong-import-order, wrong-import-position
_LOGGER = logging.getLogger(__name__)
+_RETRYABLE_REASONS = [
+ "rateLimitExceeded", "quotaExceeded", "internalError", "backendError"
+]
+_RETRYABLE_TYPES = (
Review Comment:
We have tests in this repo that run in environments where these libraries are not available. I'll rewrite it in a more clean way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1746883743
## [Codecov](https://app.codecov.io/gh/apache/beam/pull/28820?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#28820](https://app.codecov.io/gh/apache/beam/pull/28820?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e31b93b) into [master](https://app.codecov.io/gh/apache/beam/commit/95af465f0dfaa123364e5880bccc3dff32a19b39?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (95af465) will **decrease** coverage by `0.01%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #28820 +/- ##
==========================================
- Coverage 72.22% 72.22% -0.01%
==========================================
Files 684 684
Lines 101230 101230
==========================================
- Hits 73111 73109 -2
- Misses 26541 26543 +2
Partials 1578 1578
```
| [Flag](https://app.codecov.io/gh/apache/beam/pull/28820/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [python](https://app.codecov.io/gh/apache/beam/pull/28820/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `82.76% <ø> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files](https://app.codecov.io/gh/apache/beam/pull/28820?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [sdks/python/apache\_beam/io/gcp/bigquery\_tools.py](https://app.codecov.io/gh/apache/beam/pull/28820?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2JpZ3F1ZXJ5X3Rvb2xzLnB5) | `75.00% <ø> (ø)` | |
... and [2 files with indirect coverage changes](https://app.codecov.io/gh/apache/beam/pull/28820/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #28820:
URL: https://github.com/apache/beam/pull/28820#issuecomment-1746856471
Run Python 3.10 PostCommit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28820:
URL: https://github.com/apache/beam/pull/28820#discussion_r1445315150
##########
sdks/python/apache_beam/utils/retry.py:
##########
@@ -166,17 +168,38 @@ def retry_on_server_errors_and_timeout_filter(exception):
def retry_on_server_errors_timeout_or_quota_issues_filter(exception):
- """Retry on server, timeout and 403 errors.
+ """Retry on server, timeout, 429, and some 403 errors.
- 403 errors can be accessDenied, billingNotEnabled, and also quotaExceeded,
- rateLimitExceeded."""
+ 403 errors from BigQuery include both non-transient (accessDenied,
+ billingNotEnabled) and transient errors (rateLimitExceeded).
+ Only retry transient errors."""
if HttpError is not None and isinstance(exception, HttpError):
- if exception.status_code == 403:
+ if exception.status_code == 429:
return True
+ if exception.status_code == 403:
+ try:
+ # attempt to extract the reason and check if it's retryable
+ content = exception.content
+ if not isinstance(content, dict):
+ content = json.loads(exception.content)
+ return content["error"]["errors"][0]["reason"] in _RETRYABLE_REASONS
Review Comment:
Thanks for the detail. Anyways this is wrapped in a a fail-safe try clause so LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Python BQ] Retry get_table for quota errors [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 merged PR #28820:
URL: https://github.com/apache/beam/pull/28820
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org