Skip to content
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

Merged
merged 5 commits into from
Mar 1, 2024

Conversation

alextwoods
Copy link
Contributor

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.

@@ -25,6 +25,7 @@ class Configuration
DEFAULTS = {
max_messages: 10,
shutdown_timeout: 15,
retry_standard_errors: true,
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@alextwoods alextwoods merged commit a81699b into main Mar 1, 2024
39 checks passed
@alextwoods alextwoods deleted the retry_standard_errors branch March 1, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQS ActiveJobs are retried if uncaught exceptions are raised or retry attempts are exceeded
2 participants