Skip to content

feat(filecache): Scale DB query created when deleting file from filecache #54277

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat(filecache): Scale DB query created when deleting file from filecache #54277

wants to merge 2 commits into from

Conversation

CarlSchwan
Copy link
Member

Instead of creating a CacheEntryRemovedEvent for each deleted files, create a single CacheEntriesRemovedEvent which wrap multiple CacheEntryRemovedEvent.

This allow listener to optimize the query they do when multiple files are deleted at the same time (e.g. when deleting a folder).

Summary

Instead of creating a CacheEntryRemovedEvent for each deleted files, create a single CacheEntriesRemovedEvent which wrap multiple CacheEntryRemovedEvent.

This allow listener to optimize the query they do when multiple files are deleted at the same time (e.g. when deleting a folder).

For a folder with 10 files inside, the number of query created by the DELETE http request went from 72-75 to 42

image

Checklist

@CarlSchwan CarlSchwan requested a review from a team as a code owner August 6, 2025 13:25
@CarlSchwan CarlSchwan requested review from artonge, nfebe, provokateurin and yemkareems and removed request for a team August 6, 2025 13:25
@CarlSchwan CarlSchwan added this to the Nextcloud 32 milestone Aug 6, 2025
@CarlSchwan CarlSchwan force-pushed the combine-query-cache-entry-deleted branch from 1a92626 to b10e23e Compare August 6, 2025 13:34
@CarlSchwan CarlSchwan mentioned this pull request Aug 6, 2025
Closed
@CarlSchwan CarlSchwan force-pushed the combine-query-cache-entry-deleted branch from b10e23e to 12ea3fe Compare August 6, 2025 13:41
@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Aug 6, 2025
@CarlSchwan CarlSchwan self-assigned this Aug 6, 2025
@CarlSchwan CarlSchwan force-pushed the combine-query-cache-entry-deleted branch 2 times, most recently from d04ff37 to ba2ef69 Compare August 7, 2025 10:06
@CarlSchwan CarlSchwan requested a review from come-nc August 7, 2025 10:14
artonge
artonge reviewed Aug 19, 2025
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense. I guess we could also deprecate the original one to push dev to migrate to the new event ?

@github-actions GitHub Actions
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

come-nc
come-nc reviewed Aug 21, 2025
View reviewed changes
@CarlSchwan CarlSchwan force-pushed the combine-query-cache-entry-deleted branch from ba2ef69 to 211ab28 Compare August 21, 2025 15:04
@CarlSchwan
Copy link
Member Author

It makes sense. I guess we could also deprecate the original one to push dev to migrate to the new event ?

Considering that CacheEntriesRemovedEvent uses CacheEntryRemovedEvent, that might create some issues. If we deprecate the old event, we should then create a new class to store this information

This was referenced Aug 22, 2025
Merged
Merged
@CarlSchwan CarlSchwan force-pushed the combine-query-cache-entry-deleted branch from 211ab28 to 505aa5f Compare August 25, 2025 13:05
@CarlSchwan CarlSchwan marked this pull request as draft August 25, 2025 14:40
@CarlSchwan CarlSchwan mentioned this pull request Aug 25, 2025
Open
@CarlSchwan CarlSchwan force-pushed the combine-query-cache-entry-deleted branch from 505aa5f to 281a304 Compare August 25, 2025 22:35
@CarlSchwan CarlSchwan marked this pull request as ready for review August 25, 2025 22:55
This was referenced Aug 28, 2025
Merged
Merged
@provokateurin
Copy link
Member

I'm a bit worried that some apps will not listen to this new event and we break their functionality 🙈
I would prefer if this is only merged for Nextcloud 33, so developers have more time to detect this problem if it affects their apps.

@CarlSchwan
Copy link
Member Author

I'm a bit worried that some apps will not listen to this new event and we break their functionality 🙈 I would prefer if this is only merged for Nextcloud 33, so developers have more time to detect this problem if it affects their apps.

The old event is still being sent and apps need to listen to either of them and not both

@provokateurin
Copy link
Member

Ah ok, I thought you were replacing it entirely. So it just optimizes the listeners, not the event emitting.

salmart-dev
salmart-dev requested changes Sep 3, 2025
View reviewed changes
Copy link
Contributor

@salmart-dev salmart-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have minor comments, feel free to resolve them if you address them or don't want to.

lib/private/Files/Cache/Cache.php
$cacheEntryRemovedEvents[] = $cacheEntryRemovedEvent;
$this->eventDispatcher->dispatchTyped($cacheEntryRemovedEvent);
}
$this->eventDispatcher->dispatchTyped(new CacheEntriesRemovedEvent($cacheEntryRemovedEvents));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: if an error happens in any app handling the previous per-item events, the core will never get a chance to handle this event leaving the data around. This is more related to handling events in code, rather than this PR itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to send the per-item events after the grouped-item event

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is worse, it means no events gets emitted if there is an issue halway?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CarlSchwan if you want to keep the per-item event handling after the grouped one, to avoid what Côme mentions, maybe you can wrap the first with a try-catch(Exception), so at least we can try emitting the single ones? The same could be done in reverse, I guess.

Anyways the topic of "what happens when a request crashes" during/before event handling is something that cannot be solved in this PR specifically, I just wanted to raise the point that this may lead to issues and unfortunately there is no easy way around it.

@salmart-dev
Copy link
Contributor

🤦‍♂️ didn't see that there already was a second review

@CarlSchwan CarlSchwan force-pushed the combine-query-cache-entry-deleted branch from 56879c4 to 3806553 Compare September 3, 2025 11:40
Carl Schwan added 2 commits September 3, 2025 14:29
@CarlSchwan
feat(filecache): Scale DB query created when deleting file from filec…
8428bd6 
…ache

Instead of creating a CacheEntryRemovedEvent for each deleted files,
create a single CacheEntriesRemovedEvent which wrap multiple
CacheEntryRemovedEvent.

This allow listener to optimize the query they do when multiple files
are deleted at the same time (e.g. when deleting a folder).

Signed-off-by: Carl Schwan <carl.schwan@nextclound.com>
@CarlSchwan
perf(metadata): Add optimized sharding for metadata deletion
a7bb6fb 
Signed-off-by: Carl Schwan <carl.schwan@nextclound.com>
@CarlSchwan CarlSchwan force-pushed the combine-query-cache-entry-deleted branch from 3806553 to a7bb6fb Compare September 3, 2025 12:29
This was referenced Sep 4, 2025
Merged
Merged
@nextcloud-bot nextcloud-bot mentioned this pull request Sep 18, 2025
Merged
This was referenced Sep 25, 2025
Merged
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@icewind1991 icewind1991 icewind1991 left review comments

@artonge artonge artonge left review comments

@come-nc come-nc come-nc approved these changes

@provokateurin provokateurin provokateurin approved these changes

@nfebe nfebe Awaiting requested review from nfebe nfebe is a code owner automatically assigned from nextcloud/server-backend

@yemkareems yemkareems Awaiting requested review from yemkareems yemkareems is a code owner automatically assigned from nextcloud/server-backend

@salmart-dev salmart-dev Awaiting requested review from salmart-dev

Requested changes must be addressed to merge this pull request.

Assignees

@CarlSchwan CarlSchwan

Labels

3. to review Waiting for reviews feedback-requested performance 🚀

Projects

None yet

Milestone

Nextcloud 33

Development

Successfully merging this pull request may close these issues.

Un-optimized and buggy deletion of metadata

7 participants

@CarlSchwan @provokateurin @salmart-dev @icewind1991 @artonge @come-nc @skjnldsv