-
Notifications
You must be signed in to change notification settings - Fork 161
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
[Bug] Use bigquery default retryable exceptions #1431
Conversation
…lts; keep BadRequest as retryable as that is not in BigQuery's defaults and could cause a breaking change by removing it
): | ||
return True | ||
elif isinstance(error, Forbidden) and any( | ||
e["reason"] == "rateLimitExceeded" for e in error.errors |
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 condition is caught in the default retryable reasons (for more than just Forbidden
).
def _is_retryable(error: Exception) -> bool: | ||
"""Return true for errors that are unlikely to occur again if retried.""" | ||
if isinstance( | ||
error, (BadGateway, BadRequest, ConnectionError, ConnectionResetError, ServerError) |
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.
Only BadRequest
is missing from BQ's defaults; the rest can be removed since they would be caught in _job_should_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.
We determined we can remove BadRequest
here. The rationale is that we would expect a bad request to always be a bad request since the issue originates from the user side and not the BQ side.
from google.api_core.future.polling import DEFAULT_POLLING | ||
from google.api_core.retry import Retry | ||
from google.cloud.bigquery.retry import DEFAULT_RETRY | ||
from google.cloud.exceptions import BadGateway, BadRequest, ServerError | ||
from google.cloud.bigquery.retry import DEFAULT_RETRY, _job_should_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.
While this is a private function, there is no reasonable way to capture BQ's defaults here. We need to add an exception into list that goes into this function, which would then require vendoring the whole function. Even if we remove BadRequest
, we would then need to pull a private attribute (_predicate
) off of DEFAULT_JOB_RETRY
so that we can implement the retry cap (job_retries
).
@@ -65,7 +65,9 @@ def test_is_retryable(self): | |||
service_unavailable_error = exceptions.ServiceUnavailable("service is unavailable") | |||
|
|||
self.assertTrue(_is_retryable(internal_server_error)) | |||
self.assertTrue(_is_retryable(bad_request_error)) | |||
self.assertFalse( |
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.
It's worth keeping this, but flipping it, to be clear that this was once supported, and no longer is supported.
* replace our custom list of retryable exceptions with BigQuery's defaults * remove BadRequest as a retryable error (cherry picked from commit 2e1a5fd)
resolves #263
Problem
We're not retrying in all scenarios that BigQuery recommends. This is causing unexpected behavior for users. This likely happened as a result of us maintaining our own list of retryable errors, which fell out of sync with BigQuery's development over time.
Solution
Instead of maintaining our own list, we should rely on BigQuery's list of retryable errors and reasons. However, we have one scenario that is not covered by BigQuery's defaults,
BadRequest
. We need to also check this scenario after checking BigQuery's defaults to maintain backwards compatibility.Checklist