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 option to exclude locked jobs (i.e. currently being processed) in jobs:check. #1087

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joshuapinter
Copy link

@joshuapinter joshuapinter commented May 9, 2019

If a job is currently being processed, we want an option to exclude them from being reported in jobs:check.

jobs:check’s primary purpose should be to see the outstanding queue, not determining long-running jobs.

Made this option default to false so no breaking changes but simply pass true for not_locked to exclude them from this check.

Fixes #1127

@joshuapinter joshuapinter force-pushed the ignore_active_jobs_in_check_task branch 2 times, most recently from 0efa8f2 to 97e288c Compare December 1, 2020 22:43
@joshuapinter
Copy link
Author

Ping! Any interest in getting this merged? If so, what do I need to do to move it forward? Write some tests, changelog, etc.?

Let me know and I'll get on it. I would like to get back to the main fork.

Thanks!

@joshuapinter joshuapinter changed the title Add option to exclude locked jobs (i.e. currently being processed) in… Add option to exclude locked jobs (i.e. currently being processed) in jobs:check. Dec 2, 2020
@albus522
Copy link
Member

albus522 commented Dec 2, 2020

I posted a reply last night, but apparently it failed to actually submit, that's annoying.

Excluding running jobs is probably the right approach, but locked_at NULL isn't quite right. If a job is locked in a loop killing off workers, this task should probably also kick out an error. Using ready_ro_run probably carries the intention a bit better. https://github.com/collectiveidea/delayed_job_active_record/blob/master/lib/delayed/backend/active_record.rb#L55 This wouldn't be perfect as a worker could pick up the job after the lock expires and crash potentially keeping the issue quiet, but it is slightly better than only checking locked_at is set. The worker name argument can be any string that doesn't collide with a real worker name, "running job:check" would probably do.

And while not this PR's problem, it turns out when jobs:check was added, it was kind of half baked. For one, this is a very ActiveRecord specific implementation which should have been contained in https://github.com/collectiveidea/delayed_job_active_record. The task is ok here, but most of the implementation should have been over there. While I am not sure any of the other backends are actually maintained, it would have been better to continue the pattern. And created_at was the wrong marker for sitting around too long. DJ allows scheduling future jobs and this check would trip over those. run_at would be better.

… jobs:check.

If a job is currently being processed, we want an option to exclude them from being reported in `jobs:check`.

`jobs:check`’s primary purpose should be to see the outstanding queue, not determining long-running jobs.

Made this option default to false so no breaking changes but simply pass true for `not_locked` to exclude them from this check.

Remove extra `AND`.
If there are scheduled jobs in the future, they should not be included in the jobs:check, which is intended to look for jobs that should be processed but have not yet been processed.
@joshuapinter joshuapinter force-pushed the ignore_active_jobs_in_check_task branch from 3b80b25 to 4cbb738 Compare August 24, 2022 17:02
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.

Add option to exclude currently locked jobs in jobs:check.
2 participants