Page MenuHomePhabricator

Allow worker tasks to have priorities
ClosedPublic

Authored by joshuaspence on Jul 10 2014, 12:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 5:30 AM
Unknown Object (File)
Feb 17 2024, 5:35 AM
Unknown Object (File)
Feb 17 2024, 5:35 AM
Unknown Object (File)
Feb 17 2024, 5:35 AM
Unknown Object (File)
Feb 17 2024, 5:35 AM
Unknown Object (File)
Feb 11 2024, 3:30 PM
Unknown Object (File)
Feb 8 2024, 11:28 AM
Unknown Object (File)
Feb 7 2024, 6:12 PM
Tokens
"Doubloon" token, awarded by chad.

Details

Summary

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

Test Plan

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

Repository
rP Phabricator
Branch
taskpriority
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1647
Build 1648: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence retitled this revision from to Allow worker tasks to have priorities.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

I haven't looked at any of the yielding stuff so far.

src/applications/daemon/worker/DummyWorker.php
1–9 ↗(On Diff #23699)

I'll obvious remove this class later (and FailureWorker too), but I'm just keeping them here for now.

This all looks good to me.

resources/sql/autopatches/20140710.workerpriority.sql
2 ↗(On Diff #23699)

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 ↗(On Diff #23699)

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)
);

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:

  1. Add priorities (i.e. don't always use the default priority)
  2. 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.

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.

joshuaspence edited edge metadata.
  • 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
epriestley edited edge metadata.

Cool, this looks correct to me.

This revision is now accepted and ready to land.Jul 11 2014, 11:20 AM
joshuaspence edited edge metadata.
  • Change date of SQL patch to reflect landing date
joshuaspence updated this revision to Diff 23765.

Closed by commit rP9a679bf3746c (authored by @joshuaspence).

Awesome! I've really wanted this :)

@cburroughs caught some issues with this. :P

src/infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php
164–166

This test works coincidentally because FIFO order and priority order are the same.

src/infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php
186

We never actually made this order by priority.