Deletion is a possibly time-intensive process, especially with large
files that are backed by high-latency, chunked storage (such as
S3). Even ~200mb objects take minutes to delete, which makes for an
unhappy experience. Fixes T10828.
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T10828: File deletion should be queued up and run by daemons
- Commits
- rPbe00264ae74b: Make daemons perform file deletion
Delete a large file, and stare in awe of the swiftness with
which I am redirected to the main file application.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
(This isn't done yet, because once you click Delete the daemon task isn't immediately queued up, meaning it looks like the file is still in the UI for a little while.)
Some very minor inlines, take 'em or leave 'em. None of them meaningfully impacts anything today. Some of it might have been from the change you inherited too, I didn't look at the older diff in detail.
src/applications/files/controller/PhabricatorFileDeleteController.php | ||
---|---|---|
36 | Maybe drop the priority on these to PRIORITY_BULK -- they're probably less important to complete in a timely fashion than most other tasks. Also, for completeness, specify 'objectPHID' => $file->getPHID() here. Doing so allows us to find tasks affecting a given file later. | |
src/applications/files/query/PhabricatorFileSearchEngine.php | ||
18–22 | In this case, sliiiightly cleaner to put the withIsDeleted() ("global query stuff") here instead of in buildQueryFromParameters() ("per-query custom stuff"). I don't think it matters much, but a few callsites do call newQuery() without threading the query through buildQueryFromParameters(). | |
src/applications/files/worker/FileDeletionWorker.php | ||
6 | I think this cache doesn't save us anything, and it's possible we might run into trouble with it if the same Worker is ever used to run multiple jobs. That is, it makes this code not work: $worker = new Worker(); $worker->setData('a'); $worker->execute(); $worker->setData('b'); $worker->execute(); Pretty sure we don't do that, and wouldn't really expect it to work, but maybe get rid of the cache since the best case is nothing and the worst case is a confusing surprise for someone a couple years from now. | |
13 | This is extremely minor, but consider using filePHID instead of fileID -- the major benefit is that it's a little easier to debug stuff because PHIDs can go into more tools (like bin/remove destroy, global search, etc). |