-
Notifications
You must be signed in to change notification settings - Fork 159
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
Changes from all commits
89766ae
5900683
b96493a
0d4994c
a17b060
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: Fixes | ||
body: Fix retry scenarios so that dbt always retries when BigQuery recommends a retry | ||
time: 2024-12-11T14:47:52.36905-05:00 | ||
custom: | ||
Author: mikealfare | ||
Issue: "263" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,8 @@ | ||
from typing import Callable, Optional | ||
|
||
from google.api_core.exceptions import Forbidden | ||
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 | ||
from requests.exceptions import ConnectionError | ||
|
||
from dbt.adapters.contracts.connection import Connection, ConnectionState | ||
|
@@ -83,7 +81,7 @@ def __call__(self, error: Exception) -> bool: | |
self._error_count += 1 | ||
|
||
# if the error is retryable, and we haven't breached the threshold, log and continue | ||
if _is_retryable(error) and self._error_count <= self._retries: | ||
if _job_should_retry(error) and self._error_count <= self._retries: | ||
_logger.debug( | ||
f"Retry attempt {self._error_count} of {self._retries} after error: {repr(error)}" | ||
) | ||
|
@@ -113,16 +111,3 @@ def on_error(error: Exception): | |
raise FailedToConnectError(str(e)) | ||
|
||
return on_error | ||
|
||
|
||
def _is_retryable(error: Exception) -> bool: | ||
mikealfare marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"""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 commentThe reason will be displayed to describe this comment to others. Learn more. Only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We determined we can remove |
||
): | ||
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 commentThe 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 |
||
): | ||
return True | ||
return False |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,7 +53,7 @@ def generate_connection_reset_error(): | |
assert new_mock_client is not self.mock_client | ||
|
||
def test_is_retryable(self): | ||
_is_retryable = dbt.adapters.bigquery.retry._is_retryable | ||
_is_retryable = google.cloud.bigquery.retry._job_should_retry | ||
exceptions = dbt.adapters.bigquery.impl.google.cloud.exceptions | ||
internal_server_error = exceptions.InternalServerError("code broke") | ||
bad_request_error = exceptions.BadRequest("code broke") | ||
|
@@ -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 commentThe 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. |
||
_is_retryable(bad_request_error) | ||
) # this was removed after initially being included | ||
self.assertTrue(_is_retryable(connection_error)) | ||
self.assertFalse(_is_retryable(client_error)) | ||
self.assertTrue(_is_retryable(rate_limit_error)) | ||
|
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 ofDEFAULT_JOB_RETRY
so that we can implement the retry cap (job_retries
).