-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat(preview): Expire previews #55632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
53c1edc to
fce9e12
Compare
|
How are the actual preview files deleted? |
|
Right, I forgot something, unless this is somehow done already 😂 |
Signed-off-by: provokateurin <kate@provokateurin.de>
Signed-off-by: provokateurin <kate@provokateurin.de>
fce9e12 to
fee65e5
Compare
|
Now :) |
|
|
||
| public function deleteExpiredPreviews(int $maxAgeDays): void { | ||
| $lastId = 0; | ||
| while (true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do everything at once? If an admin suddenly sets preview_expiration_days to a very low value, there will need a lot of work to delete everything, so the cron might timeout at some point.
The fact that the background job runs regularly should be enough to lower the number of previews little by little. And we can also add a command to cleanup everything that won't be limited if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but if it is limited to some value per run, then the job might not be able to catch up with deleting all expired previews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sensitive defaults should be acceptable. Maybe increase the job frequency to make sure this works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A common pattern we have elsewhere is to limit by time. e.g. max 1 hour running
$startTime = time();
while (true) {
$done = $this->doTheStuffWithLimit(1000);
if ($done) {
break;
}
// Stop if execution time is more than one hour.
if (time() - $startTime > 3600) {
return;
}
| while (true) { | ||
| $previews = $this->previewMapper->getPreviews($lastId, PreviewMapper::MAX_CHUNK_SIZE, $maxAgeDays); | ||
| $i = 0; | ||
| foreach ($previews as $preview) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make sense to wrap this in a transation and delete by chunk to speed up a bit the process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I just copied from the deleteAll method which doesn't have that (probably because it will only be used by tests?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's currently only used in tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm but with a transaction the DB and storage could get out of sync and I don't know how that is handled. What happens if the files are deleted, but the entries are still in the DB?
(Ok they can get out of sync without a transaction as well, but it would be limited to a single preview and not all expired ones)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't call rollback but just commit if something fails in the middle while removing a preview from the storage?
Closes #45962
This is disabled by default, to avoid potentially heavy load for servers which need to regenerate previews. Admins should only enable this is if storage usage is a concern and they know which expiration threshold is good for their use-case.