Skip to content
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

RFC: Cache locally the filecache information #31793

Closed
wants to merge 1 commit into from

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Apr 1, 2022

This on one side might improve the performance by doing less requests to
the database but more importantly this decrease the likeliness of read
after write errors in a cluster setup since we don't requests the data
from the database that just got updated

Currently this only cache get/put/insert/update and for
copy/move/propagator this clear the cache.

Signed-off-by: Carl Schwan carl@carlschwan.eu

@CarlSchwan CarlSchwan added the 2. developing Work in progress label Apr 1, 2022
@CarlSchwan CarlSchwan self-assigned this Apr 1, 2022
@CarlSchwan CarlSchwan marked this pull request as draft April 1, 2022 17:53
This on one side might improve the performance by doing less requests to
the database but more importantly this decrease the likeliness of read
after write errors in a cluster setup since we don't requests the data
from the database that might be outdated.

Currently this only cache get/put/insert/update and for
copy/move/propagator this clear the cache.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan CarlSchwan force-pushed the performance/local-cache-filecache branch from 6d38880 to a8f72a9 Compare April 3, 2022 11:03
@CarlSchwan
Copy link
Member Author

CarlSchwan commented Apr 4, 2022

Remaining issues are mostly with the unit tests of the sharing and versions apps that directly do queries to the filecache table :(

@CarlSchwan CarlSchwan changed the title Cache locally the filecache information RFC: Cache locally the filecache information Apr 4, 2022
@CarlSchwan
Copy link
Member Author

@skjnldsv @icewind1991 What do you think of this?

Comment on lines +313 to +323
$values['fileid'] = $fileId;
$valuesDefault = [
'encrypted' => 0,
'storage_mtime' => 0,
'permissions' => 0,
'etag' => null,
'checksum' => null,
'metadata_etag' => null,
'creation_time' => null,
'upload_time' => null,
];
Copy link
Member

Choose a reason for hiding this comment

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

This is the only part I don't like.
Hardcoded values? Do we have a single point of truth for what can be populated as file data? Is this not dynamic?
Any reason we only store those ones?

As a summary, could you give a little background on how you found that specific list? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is basically the default value coming from the database, I don't like that we duplicate it either

Currently, we basically create an entry in the DB with partial data, then the missing data is filled by the DB with the default value and then we fetch the object from the DB.

@blizzz blizzz added this to the Nextcloud 25 milestone Apr 21, 2022
@PVince81
Copy link
Member

will you finish this ?

please consult with @icewind1991 also

@icewind1991
Copy link
Member

Currently this will lead to state size information for folders if it's contents have been modified.

In general I'm very hesitant to do this due to reasons like this, it might be fine with proper pessimistic cache invalidation

@skjnldsv skjnldsv mentioned this pull request Aug 18, 2022
This was referenced Aug 24, 2022
This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz removed this from the Nextcloud 26 milestone Mar 9, 2023
@blizzz blizzz added this to the Nextcloud 27 milestone Mar 9, 2023
This was referenced May 3, 2023
@skjnldsv skjnldsv modified the milestones: Nextcloud 27, Nextcloud 28 May 9, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv
Copy link
Member

As there is no feedback since a while I will close this ticket.
If you will decide to work on this feature again and if it hasn't been fixed or implemented already, feel free to re-open and solve the various conflicts.

@skjnldsv skjnldsv closed this Feb 27, 2024
@skjnldsv skjnldsv deleted the performance/local-cache-filecache branch February 27, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants