Page MenuHomePhabricator

Track total time from task creation to task archival
ClosedPublic

Authored by amckinley on Feb 20 2019, 7:18 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 5, 2:06 AM
Unknown Object (File)
Feb 9 2024, 2:09 PM
Unknown Object (File)
Feb 7 2024, 5:03 PM
Unknown Object (File)
Jan 19 2024, 4:38 PM
Unknown Object (File)
Jan 15 2024, 5:03 PM
Unknown Object (File)
Jan 11 2024, 8:27 PM
Unknown Object (File)
Jan 7 2024, 4:11 PM
Unknown Object (File)
Jan 7 2024, 4:24 AM
Subscribers
Restricted Owners Package

Details

Summary

Ref T5401. Depends on D20201. Add timestamps to worker tasks to track task creation, and pass that through to archive tasks. This lets us measure the total time the task spent in the queue, not just the duration it was actually running.

Also displays this information in the daemon status console; see screenshot:

Screen Shot 2019-02-20 at 2.18.30 PM.png (266×1 px, 38 KB)

Test Plan

Stopped daemons, ran bin/search index --all --background to create lots of tasks, restarted daemons, observed expected values for dateCreated and epochArchived in the archive worker table.

Also tested the changes to unarchiveTask by forcing a search task to permanently fail and then bin/worker retrying it.

Diff Detail

Repository
rP Phabricator
Branch
daemon-completed
Lint
Lint Errors
SeverityLocationCodeMessage
Error/Users/amckinley/src/phacility/core/lib/libphutil/src/filesystem/Filesystem.php:428PHL1Unknown Symbol
Error/Users/amckinley/src/phacility/core/lib/libphutil/src/filesystem/Filesystem.php:568PHL1Unknown Symbol
Unit
Tests Passed
Build Status
Buildable 22089
Build 30174: Run Core Tests
Build 30173: arc lint + arc unit

Event Timeline

Owners added a subscriber: Restricted Owners Package.Feb 20 2019, 7:18 PM

I actually ran the 2nd migration when the table was full of active tasks and nothing broke. We could try to get clever by setting dateCreated and dateModified for active tasks to now(), but in the worst-case scenario where we end up doing some math wrong and show negative 2 billion seconds spent waiting in the queue, it'll fix itself 14 days after this deploys.

epriestley added inline comments.
src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
121–122

PhabricatorWorkerArchiveTask->unarchiveTask() might need a similar change (copy over datecreated / datemodified, but just drop archivedEpoch since it's no longer archived).

This is currently only testable by bin/worker retry-ing a permanently failed task, but I want to change bin/worker retry to let you retry successful tasks too, just with a warning ("This task already succeeded, so retrying it might do whatever it does twice, are you sure you can handle that?")

This revision is now accepted and ready to land.Feb 20 2019, 7:21 PM
src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
121–122

This doesn't quite work. dateCreated can be copied over fine, but Lisk is (I assume) detecting that the object is dirty and overriding my dateModified to be the current time.

src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
121–122

Yeah, it's willSaveObject on LiskDAO. I could implement willSaveObject on WorkerActiveTask and just do the right thing, unless you have another idea.

I think it's fine to let dateModified update to the current time -- you're technically modifying the thing.

Update unarchive, add code to daemon console to use this new data.

amckinley edited the test plan for this revision. (Show Details)

Can you take a quick look at this with the added UI changes?

Looks good to me. I'll see about getting random_bytes and random_int into the builtins map to fix that lint thing.

src/applications/daemon/controller/PhabricatorDaemonConsoleController.php
52

(This needs a libphutil/ patch too, right?)

src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
122

If the other setDateModified() didn't work, this one probably has no effect either?

This revision is now accepted and ready to land.Feb 20 2019, 10:29 PM
amckinley added inline comments.
src/applications/daemon/controller/PhabricatorDaemonConsoleController.php
52

Yep, D20201 just landed.

src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
122

Oh yeah, it'll be overwritten the same way.

This revision was automatically updated to reflect the committed changes.
amckinley marked 2 inline comments as done.