Page MenuHomePhabricator

Make daemons perform file deletion
ClosedPublic

Authored by amckinley on Apr 18 2016, 12:41 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 1, 12:56 AM
Unknown Object (File)
Feb 21 2024, 7:19 AM
Unknown Object (File)
Feb 3 2024, 7:41 PM
Unknown Object (File)
Feb 3 2024, 7:41 PM
Unknown Object (File)
Feb 3 2024, 7:41 PM
Unknown Object (File)
Feb 3 2024, 9:49 AM
Unknown Object (File)
Feb 3 2024, 9:49 AM
Unknown Object (File)
Feb 3 2024, 9:49 AM

Details

Summary

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.

Test Plan

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
Branch
T10828-daemonize-file-deletion
Lint
Lint Passed
Unit
Tests Skipped
Build Status
Buildable 11812
Build 22051: Run Core Tests
Build 14816: Run Core Tests
Build 14815: arc lint + arc unit

Event Timeline

thoughtpolice retitled this revision from to Make daemons perform file deletion.
thoughtpolice updated this object.
thoughtpolice edited the test plan for this revision. (Show Details)

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

thoughtpolice edited edge metadata.

Fix usage of public vs protected

  • Mark file metadata as 'deleted' in controller
  • Add an isDeleted column and update important controllers to check for it

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
35

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–20 ↗(On Diff #42620)

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

This revision is now accepted and ready to land.Apr 18 2017, 6:00 PM
This revision was automatically updated to reflect the committed changes.