Page MenuHomePhabricator

Merge the "Herald" and "Owners" daemon workers into a single "Publish" worker
ClosedPublic

Authored by epriestley on Apr 23 2019, 4:55 PM.

Details

Summary

Depends on D20466. Ref T13277. Currently:

  • The "Owners" worker writes ownership relationships (e.g., commit X affects package Y, because it touches a path in package Y) -- these are just edges.
  • It also triggers audits.
  • Then it queues a "Herald" worker.
  • This formally publishes the commit and triggers Herald.

These aren't really separate steps and can happen more easily in one shot. Merge them.

Test Plan
  • Ran bin/repository reparse --publish to republish various commits, got sensible behavior.
  • Grepped for "IMPORTED_OWNERS", "IMPORTED_HERALD", "--herald", "--owners", and "--force-local" flags.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Apr 23 2019, 4:55 PM
epriestley requested review of this revision.Apr 23 2019, 4:56 PM
amckinley accepted this revision.Apr 24 2019, 4:10 AM
amckinley added inline comments.
src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php
60

'Publish changes. Includes evaluating Owners packages and Herald rules.'?

117–121

Never saw control flow like this before. Makes sense, but it took me a second.

124–125

Extra "or"

125

"; or --importing for all uncompleted steps"

223–229

I think it's a little unfair to remove this and the similar warning back on line 22.

src/applications/repository/storage/PhabricatorRepositoryCommit.php
36–37

What happens to commits currently sitting around with importStatus IN (IMPORTED_OWNERS, IMPORTED_HERALD) when this update gets applied? I guess we'll just end up running the publish step again, which is fine?

37

Don't we need a migration for this? Won't the DB suddenly have a bunch of old PhabricatorRepositoryCommit with importStatus = 15 and a bunch of new ones with importStatus = 11?

src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php
12

I can see that this comes from PhabricatorRepositoryCommitHeraldWorker, but maybe it's a good reason to separate these workers?

This revision is now accepted and ready to land.Apr 24 2019, 4:10 AM

I think it's a little unfair to remove this and the similar warning [about running the daemons] back on line 22.

I think both of these warnings date from a time when running the daemons was optional. Long ago, we'd send email in-process if you weren't running the daemons, and most of the other stuff that only happens in the daemons didn't exist yet, so you could have an almost-fully-functional install with no daemons. Both messages are calling the queueing behavior out as exceptional in case you aren't running taskmaster daemons.

Now, daemons are mandatory and a lot of stuff (email, feed, repositories) simply doesn't work if you don't run them so I think these warnings are probably no longer useful.

Particularly, this mimics bin/search index [--background], which I don't recall seeing confusion over.

I can see that this comes from PhabricatorRepositoryCommitHeraldWorker, but maybe it's a good reason to separate these workers?

Is there a specific reason this motivates separation (e.g., a chain of events where separating them leads to a better outcome than combining them, specifically because part of the workflow may be slow)? The only thing that springs to mind is that if the "Owners" step has some kind of call which is likely to hang/time out, we'll kill it sooner if it's separate -- but it doesn't have anything like that AFAIK, and killing it sooner doesn't really matter much since it will just retry anyway.

(Separating them also lets us apply owners/audit changes as soon as they're ready, but I think this is actually sort of a bad thing: if there's some kind of issue with pulling the diff, it's probably better that the whole commit stall than that half of the process work.)

epriestley updated this revision to Diff 48842.Thu, Apr 25, 4:37 PM
  • Improve help text.
epriestley added inline comments.Thu, Apr 25, 4:43 PM
src/applications/repository/storage/PhabricatorRepositoryCommit.php
36–37

If the next queued import step for a commit is:

  • "Message" or "Change": Things will work fine, they'll just move from "Change" to "Publish".
  • "Owners" or "Herald": The task classes no longer exist, so these will tasks will permanently. The commits will never fully import. Administrators will need to bin/repository reparse if they want these steps to finish.

I'll call this out in the changelog ("Finish importing any outstanding repositories before upgrading"), but even if users ignore this advice the number of impacted commits should be small, and skipping some parse steps for a handful of commits normally isn't a problem.

37

Older commits will have an extra bit (4) in the bitfield, but that doesn't hurt anything. All tests against the bitfield examine specific bits and this field is purely internal.

For the sake of cleanliness, we could wipe this bit from memory by setting the field to field & ~4, but that seems fairly low-value and would make reverting this change more difficult/dangerous.

so these will tasks will permanently

Er, these tasks will fail permanently.

This revision was automatically updated to reflect the committed changes.