Page MenuHomePhabricator

Allow worker tasks to have priorities
ClosedPublic

Authored by joshuaspence on Jul 10 2014, 12:27 PM.

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
Lint
Lint Skipped
Unit
Unit Tests Skipped

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.

epriestley edited edge metadata.Jul 10 2014, 1:23 PM

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.

joshuaspence added inline comments.Jul 10 2014, 1:37 PM
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 accepted this revision.Jul 11 2014, 11:20 AM
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 the test plan for this revision. (Show Details)Jul 11 2014, 3:25 PM
joshuaspence edited edge metadata.
  • Change date of SQL patch to reflect landing date
joshuaspence closed this revision.Jul 11 2014, 5:02 PM
joshuaspence updated this revision to Diff 23765.

Closed by commit rP9a679bf3746c (authored by @joshuaspence).

Awesome! I've really wanted this :)

chad awarded a token.Jul 11 2014, 6:25 PM

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