Page MenuHomePhabricator

Add a flag to ./bin/worker to select tasks based on their failureCount
ClosedPublic

Authored by jcox on Nov 21 2016, 7:04 PM.
Tags
None
Referenced Files
F12816944: D16906.id40708.diff
Thu, Mar 28, 3:59 AM
F12816943: D16906.id40707.diff
Thu, Mar 28, 3:59 AM
F12816942: D16906.id40704.diff
Thu, Mar 28, 3:58 AM
F12816941: D16906.id.diff
Thu, Mar 28, 3:58 AM
Unknown Object (File)
Thu, Mar 21, 2:13 PM
Unknown Object (File)
Feb 9 2024, 1:53 PM
Unknown Object (File)
Jan 31 2024, 3:09 PM
Unknown Object (File)
Jan 22 2024, 10:05 PM

Details

Summary

I frequently run into a situation where I want to kill tasks that have accumulated a lot of failures regardless of what class they are. Or I'll want to kill every worker of a certain class but only if it has failed at least once. This change allows me to run ./bin/worker cancel --class <MYCLASS> --min-failure-count 5 to only kill tasks with at least 5 failed attempts.

The --min-failure-count N argument can be used by itself as well as with --class CLASSNAME. I don't think it makes sense for it to work with --id ID, but I'm not dead set on that or anything.

Test Plan

I ran the worker management workflow with and without the --min-failure-count argument and it worked as expected.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jcox retitled this revision from to Add a flag to ./bin/worker to select tasks based on their failureCount.
jcox updated this object.
jcox edited the test plan for this revision. (Show Details)
epriestley added a reviewer: epriestley.
epriestley added inline comments.
src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php
32–35

We should probably just accept all selectors and apply them with AND, like UI queries do, although I guess the fact that we aren't using a real Query object further down makes this harder.

That is, doesn't seem inherently wrong to accept --id 1 --id 2 --id 3 --class X as "all tasks with class X and IDs 1, 2 or 3".

Not a big deal either way.

src/infrastructure/daemon/workers/query/PhabricatorWorkerArchiveTaskQuery.php
4

For YAGNI-future-proofing, maybe: withFailureCountBetween($min, $max)

Where both are nullable, and null means "no boundary limit".

Similar to existing withVersionBetween(), withNextEventBetween(), etc.

5–6

4-space indents? THE WORK OF THE DEVIL

This revision is now accepted and ready to land.Nov 21 2016, 11:22 PM
jcox edited edge metadata.

Backed PhabricatorWorkerActiveTasks with an actual query object and refactored to share
some logic between Active and Archived tasks. Also removed some overeager indentations.

jcox requested a review of this revision.Nov 22 2016, 2:03 AM
jcox edited edge metadata.

@epriestley do these changes look alright? I pulled most of PhabricatorWorkerArchiveTaskQuery up into the abstract PhabricatorWorkerTaskQuery and then added PhabricatorWorkerActiveTaskQuery. That made it more straightforward to just AND all the constraints together.

epriestley edited edge metadata.

Yep, seems reasonable to me. Thanks!

src/infrastructure/daemon/workers/query/PhabricatorWorkerTaskQuery.php
99

Maybe <= for consistency with $min and MySQL's BETWEEN?

This revision is now accepted and ready to land.Nov 22 2016, 2:03 AM
jcox edited edge metadata.

Use <= rather than <

This revision was automatically updated to reflect the committed changes.