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

Expose CLI cleanup_preserved_jobs functionality via GoodJob? #351

Closed
aried3r opened this issue Aug 26, 2021 · 1 comment
Closed

Expose CLI cleanup_preserved_jobs functionality via GoodJob? #351

aried3r opened this issue Aug 26, 2021 · 1 comment

Comments

@aried3r
Copy link
Contributor

aried3r commented Aug 26, 2021

Hey! I've just finished reading through the README and it pretty much answered all my questions. Good job (pun intended) on the documentation!

However, I was wondering if there was a way to officially expose a method to use in a rake task or job to clean up old jobs. The README suggests using:

GoodJob::Job.finished(1.day.ago).delete_all

However, the CLI task does a bit more, namely instrumenting via ActiveSupport::Notifications:

ActiveSupport::Notifications.instrument(
"cleanup_preserved_jobs.good_job",
{ before_seconds_ago: configuration.cleanup_preserved_jobs_before_seconds_ago, timestamp: timestamp }
) do |payload|
deleted_records_count = GoodJob::Job.finished(timestamp).delete_all
payload[:deleted_records_count] = deleted_records_count
end

Of course I can just copy the code, but since that functionality seems to be desired behavior since it's implemented in the CLI, maybe it's a good idea to expose this directly? Would also make it easier to schedule this via GoodJob itself.

Another question would be, is there a reason why .delete_all is enough and we don't need callbacks via .destroy_all? I assume there's just no need for them as the jobs have no associations to destroy or nullify, but just wanted to make sure.

Thanks!

@bensheldon
Copy link
Owner

@aried3r this is a great suggestion to move this into the public interface! GoodJob.cleanup_preserved_jobs sounds like a good place to extract this behavior and make it easier/safer for reuse.

is there a reason why .delete_all is enough

You're correct: there are no callbacks, so it's safe to simply delete them. And likely there never should be callbacks because this could be a fairly beefy resultset of thousands of jobs.

I support this if you wanted to submit a PR.

Would also make it easier to schedule this via GoodJob itself.

Yes, totally. Longer-term (just so you have a sense of where I imagine things heading) I think GoodJob will do have the capability to do its own housekeeping: scheduling a thread that will occasionally delete old jobs in the background, rather than burden the user with running the rake task. Your suggestion helps move in that direction 👍 This would also help unlock a request for GoodJob to preserve errored jobs by default, which I've deferred because it could potentially fill up someone's database unexpectedly if there wasn't also a default mechanism cleaning things up.

aried3r added a commit to aried3r/good_job that referenced this issue Aug 30, 2021
aried3r added a commit to aried3r/good_job that referenced this issue Aug 30, 2021
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

No branches or pull requests

2 participants