-
Notifications
You must be signed in to change notification settings - Fork 207
Support retrying when S3 deletion fails #1271
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
base: main
Are you sure you want to change the base?
Support retrying when S3 deletion fails #1271
Conversation
|
Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA. In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR. CLA has not been signed by users: @Avinash-1394 |
|
|
||
| @retry( | ||
| stop=stop_after_attempt(4), # Up to 4x boto3 retries | ||
| wait=wait_exponential(multiplier=30, min=30, max=300), # 30s, 60s, 120s, 240s |
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.
are these supposed to be hard coded? Shouldn't this use the s3_deletion_retry_* config?
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 checked for the possible connection configs here -
https://github.com/dbt-labs/dbt-adapters/blob/main/dbt-athena/src/dbt/adapters/athena/connections.py
I couldn’t find one specific for s3 retries. Should I add one there or are you referring to another config I have access to?
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.
In your PR description you mentioned that you were going to add those:
Configuration options added:
s3_deletion_retry_attempts: Number of retry attempts (default: 3)
s3_deletion_retry_base_delay: Base delay in seconds (default: 1)
s3_deletion_retry_max_delay: Maximum delay in seconds (default: 30)
are you adding those?
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.
Ah sorry for including that. I tried doing it but since the connection parameters were not available to the decorator I could not find a way to make it work. I've removed that from the description 👍🏽
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 if you nest the _delete_with_app_retry within the delete_from_s3 function it should be accessible (that's the way it's used in AthenaCursor.execute
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.
Hey just wanted to provide an update I'm trying to do this and will reach out to you over slack when I'm able to get it working with tests
resolves # #1270
docs dbt-labs/docs.getdbt.com/#
Problem
dbt-athena currently does not implement retry logic for S3 deletion operations, causing dbt runs to fail immediately when encountering transient S3 issues. This makes the adapter fragile in production environments where temporary S3 service disruptions, network connectivity issues, rate limiting, or eventual consistency problems are common.
Current problematic behavior:
Solution
This PR implements a robust retry mechanism for S3 deletion operations with the following features:
Implementation details:
Alternatives considered:
Checklist