-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add retry of additional errors #2694
Add retry of additional errors #2694
Conversation
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.
I have some suggestions on the mechanics of the error type checks (isinstance takes a tuple of types instead of requiring multiple calls), but otherwise this looks good to me!
As always, thanks for your contribution to dbt! Please remember to add yourself to the contributors list in the changelog as well.
for type in REOPENABLE_ERRORS: | ||
if isinstance(error, type): |
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.
I think you can just do if isinstance(error, REOPENABLE_ERRORS)
here, right?
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.
TIL. Done.
for error_type in RETRYABLE_ERRORS: | ||
if isinstance(error, error_type): |
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.
See above, I think this can just be if isinstance(error, RETRYABLE_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.
Done.
Thanks for the quick review! Believe I got the changes you requested @beckjake |
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.
Thanks! I'll get this merged, it'll go out in 0.18.0
resolves #1579
Description
This is a follow-up to #1963 which I've been sitting on for a while, which adds a few more errors to the class of retryable errors in BigQuery, and re-opens a connection for some that involve a broken connection. Would love to get this out of our local patches :)
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.