worker to select tasks based on their failureCount
ClosedPublic
Actions

Authored by jcox on Nov 21 2016, 7:04 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

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

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 updated this revision to Diff 40696.Nov 21 2016, 7:04 PM

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)

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptNov 21 2016, 7:04 PM

Herald added subscribers: yelirekim, epriestley. · View Herald Transcript

epriestley accepted this revision.Nov 21 2016, 11:22 PM

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

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

Herald added a subscriber: Korvin. · View Herald TranscriptNov 22 2016, 12:23 AM

@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.