-
Notifications
You must be signed in to change notification settings - Fork 62
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_standard_errors config for SQS ActiveJob #115
Conversation
@@ -25,6 +25,7 @@ class Configuration | |||
DEFAULTS = { | |||
max_messages: 10, | |||
shutdown_timeout: 15, | |||
retry_standard_errors: true, |
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.
Do we really need to preserve backwards compatibility for this? It seems safe to not rely on the fact that something failed and retried implicitly.
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 added retry_standard_errors with a default of true because right now, users are getting a built in retry of everything and they could be relying on that to retry expected, transient errors. If they set it to false (or if we default it to false), any of those errors that were previously retried will now fail immediately on the first error.
I've also added a check on execution_exceptions
and we will NOT retry if ActiveJob has already caught/handled exceptions (note, this is true both for retry_on
and discard_on
). I think this is a reasonable compromise that address the original issue while not removing existing retry behavior that users may be relying on.
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.
Ok. I think we should remove the config in a new MV? If you agree, add a TODO so we don't forget.
Issue #, if available:
Fixes #114
Description of changes:
Adds configuration to change the behavior of SQS ActiveJob's handling of retries for exceptions from ActiveJobs.
The existing behavior does not follow the [https://guides.rubyonrails.org/active_job_basics.html#retrying-or-discarding-failed-jobs](Rails ActiveJob) retry/discard behavior which notes that failed jobs are not retried unless the jobs are configured otherwise.
To avoid this being a breaking change for existing users, the flag currently defaults to
true
(preserving the existing behavior).Pending review, I will update the Readme to describe in more detail the interaction of SQS retry and Rails ActiveJob retries.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.