feat(occ): New command to delete previews #35189
Conversation
|
Seems promising. Tested also on local filesystem? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
szaimen
requested review from
danxuliu,
a team,
blizzz,
juliushaertl and
skjnldsv
and removed request for
a team
Just tested it on local storage configuration. It worked as well. |
core/Command/Preview/Delete.php
Outdated
| ->leftJoin('a', 'filecache', 'b', $qb->expr()->eq( | ||
| $qb->expr()->castColumn('a.name', IQueryBuilder::PARAM_INT), 'b.fileid' | ||
| )) | ||
| ->where($and); |
There was a problem hiding this comment.
Might still be a good idea to limit the results and rather run multiple queries after each other in a php loop to avoid massive queries against the db.
There was a problem hiding this comment.
I can first narrow down the preview search on the storage that holds the preview folder.
Then do you have in mind something like "SELECT ... FROM ... LIMIT a OFFSET a*x", with for loop on x? If so, do you know what a good batch size would be?
If the issue is not the mysql query size itself, but on the call for deletions, these deletions are actually done in a for loop in deletePreviews function (line 109).
There was a problem hiding this comment.
Yes, I was thinking about the query size itself. You wouldn't need the offset as when going through the chunk the entries would be deleted from the table so the next query with the same limit should just give you the next batch.
There was a problem hiding this comment.
I can first narrow down the preview search on the storage that holds the preview folder.
This should still be fine to keep in the query as the storage column is indexed.
There was a problem hiding this comment.
I can first narrow down the preview search on the storage that holds the preview folder.
This should still be fine to keep in the query as the storage column is indexed.
I updated the sql query with a condition on storage.
There was a problem hiding this comment.
Yes, I was thinking about the query size itself. You wouldn't need the offset as when going through the chunk the entries would be deleted from the table so the next query with the same limit should just give you the next batch.
I added an option to delete previews by batch.
Two notes:
- Batch deletion is incompatible with dry mode, as it relies on actually deleting previews (the proposed code takes care of catching these incompatible options).
- This option makes the code not in line with lib/private/Preview/BackgroundCleanupJob.php or core/Command/Preview/ResetRenderedTexts.php that do not have a batch mode.
charleypaulus
force-pushed
the
deletePreviews
branch
4 times, most recently
from
7fe61cd
to
c37a557
Compare
skjnldsv
added
2. developing
|
php-cs is not happy :) |
charleypaulus
requested review from
CarlSchwan,
ChristophWurst,
miaulalala and
nickvergessen
as code owners
This comment was marked as resolved.
This comment was marked as resolved.
You can drop them, only add changes from
It should automatically use the |
|
Hi, this PR is still active? Because i need it for fix my s3 issues 🙄 |
|
up ? |
I hope it is still active. I've been waiting for review and approval. I have fixed conflicts with latest master a few times. I can fix them again if I know there is an intention to include this PR. |
I hope too, in the meantime I deactivate previews... |
skjnldsv
added
2. developing
solracsf
changed the title
|
@charleypaulus Please fix the conflicts in this PR, it's running a little late but hopefully this will get in for NC30 |
Too late for 30. 😕 If anyone do so, please ping me directly, I'll do my best to review ans assist to get this in in our early 31 release cycle!! 💪 |
Provides a new occ command to delete previews, with options:
Tested on NC 26, with S3 as primary object storage, and with local storage. Tested all combinations of options. Verified that previews are actually deleted from S3 storage and from filecache.
Addressing these issues (as a manual command):
#26014, #20344, #18416, #35290
@szaimen, @solracsf, @arpadmuller, @skjnldsv, @SimplyCorbett, @MorrisJobke