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

Branch

BinWorkerCancelFailureParam (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 14601
Build 19060: Run Core Tests
Build 19059: arc lint + arc unit

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.