-
Notifications
You must be signed in to change notification settings - Fork 7
Move On Cancellation Logic to After Failure Hook #42
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
Conversation
Delayed::Job sets this option to true by default, you will need to configure it to false | ||
in order to include this library. |
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.
Should we include this in the README too?
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.
Added! thoughts?
In order to allow individual jobs in the JobGroup to perform cleanup activities upon job failure, a JobGroups on cancellation job must be enqueued in the after failure delayed job lifecycle hook. If both were to be performed in the same hook, the job's failure hook and the on cancellation job may run at the same time since hook execution order is never guaranteed. This is particular important if the on cancellation job depends on state set in the failure hook of an individual job. This also forces the Delayed::Worker.destroy_failed_jobs to be set to false in order to prevent a race condition where a JobGroup could be completed (instead of cancelled) if a successful job occurs at the same time as a failing job.
fad8207
to
77f8014
Compare
$ rails generate delayed_job_groups_plugin:install | ||
$ rake db:migrate | ||
|
||
## Upgrading from 0.14.0 |
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.
This is true for the initial installation too, right? If you're up for it, maybe it would be nice to include in the delayed_job_groups_plugin:install
generator?
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.
Updated the generator to set the option, let me know what you think! Had to find a work around for being able to run the generator with the gem included, since generators run after railties run... We can assume the library hasn't been fully setup until the table is present, so we can skip raising the error in that case.
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.
Looks great!
$ rails generate delayed_job_groups_plugin:install | ||
$ rake db:migrate | ||
|
||
## Upgrading from 0.14.0 |
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.
Looks great!
Addressed this issue.
Re-release #39 under a new major version
prime @jturkel @erikkessler1