Fixes T5336. Currently, PhabricatorWorkerLeaseQuery is basically FIFO. It makes more sense for the queue to be a priority-queue, and to assign higher priorities to alerts (email and SMS).
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T5336: Worker tasks should have priorities
- Commits
- Restricted Diffusion Commit
rP9a679bf3746c: Allow worker tasks to have priorities
Created dummy tasks in the queue (with different priorities). Verified that the priority field was set correctly in the DB and that the priority was shown on the /daemon/ page. Started a PhabricatorTaskmasterDaemon and verified that the higher priority tasks were executed before lower priority tasks.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- taskpriority
- Lint
Lint Passed Severity Location Code Message Advice src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php:184 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 1606 Build 1607: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
I haven't looked at any of the yielding stuff so far.
| src/applications/daemon/worker/DummyWorker.php | ||
|---|---|---|
| 1–9 | I'll obvious remove this class later (and FailureWorker too), but I'm just keeping them here for now. | |
Comment Actions
This all looks good to me.
| resources/sql/autopatches/20140710.workerpriority.sql | ||
|---|---|---|
| 2 | Unless we need BIGINT, let's just make this INT. Reading 64-bit integers from the database on 32-bit systems is probably more of a mess than "only" having a few billion priority levels. | |
| resources/sql/autopatches/20140710.workerpriority.sql | ||
|---|---|---|
| 2 | Agreed. I only set this to BIGINT because I found this patch in resources/sql/patches/005.workers.sql: create table {$NAMESPACE}_worker.worker_task ( id int unsigned not null auto_increment primary key, taskClass varchar(255) not null, leaseOwner varchar(255), leaseExpires int unsigned, priority bigint unsigned not null, failureCount int unsigned not null, key(taskClass), key(leaseOwner), key(leaseExpires) ); | |
Comment Actions
By the way, how can I test the yielding stuff? In don't really know what this is.
AFAICT, all that's left to be on this diff is:
- Add priorities (i.e. don't always use the default priority)
- Fix yielding tasks
If there's something else that would need to be done, let me know as I'll probably be able to finish this up tonight.
Comment Actions
I think this already handles yielding tasks implicitly, since the first-cut plan is to just ignore priorities when retrying.
You should be able to test them by having a DummyWorker or similar that just throws a yield exception, I think.
I'll read through T5336 and see if I can come up with anything else, but this looks complete to me.
Comment Actions
- Use int instead of bigint
- Add separate PhabricatorWorker priorities
- Use null as the default priority
- Add test cases
- Remove test classes
- Minor changes
- Mark some additional methods as final
Comment Actions
@cburroughs caught some issues with this. :P
| src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php | ||
|---|---|---|
| 164–166 ↗ | (On Diff #23765) | This test works coincidentally because FIFO order and priority order are the same. |
| src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php | ||
| 184–185 | We never actually made this order by priority. | |