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

Listen to cache event for managing metadata #34586

Closed

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Oct 13, 2022

Fix #34424
Need #34773

NodeDeletedEvent is called when a file is put to the trash, and we want to remove metadata when the file is really deleted, so we now listen to CacheEntryRemovedEvent.

  • Also, a bit of refactoring to adapt code to this change.

@artonge artonge added bug 2. developing Work in progress labels Oct 13, 2022
@artonge artonge added this to the Nextcloud 26 milestone Oct 13, 2022
@artonge artonge self-assigned this Oct 13, 2022
@artonge artonge force-pushed the artonge/fix/listen_to_cache_events_for_metadata branch from 860e4c2 to e579dfd Compare October 13, 2022 16:15
@artonge artonge marked this pull request as ready for review October 17, 2022 17:17
@artonge artonge force-pushed the artonge/fix/listen_to_cache_events_for_metadata branch from e579dfd to 7f962a3 Compare October 17, 2022 17:17
@artonge artonge requested review from a team, PVince81 and skjnldsv and removed request for a team October 17, 2022 17:17
@artonge
Copy link
Contributor Author

artonge commented Oct 17, 2022

I currently have this kind of errors when creating a new file: is locked, existing lock on file: exclusive need to check whether it is due to this PR or global.

@artonge artonge force-pushed the artonge/fix/listen_to_cache_events_for_metadata branch 2 times, most recently from e2a54a6 to f5638ca Compare October 17, 2022 17:21
@artonge artonge force-pushed the artonge/fix/listen_to_cache_events_for_metadata branch 2 times, most recently from c3fd7c2 to 11b56d0 Compare October 18, 2022 15:58
@github-actions
Copy link
Contributor

Possible performance regression detected

Show Output
4 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 5 queries added
  + SELECT "storage", "path", "mimetype" FROM "oc_filecache" WHERE "fileid" = :dcValue1
  + SELECT "storage_id", "root_id", "user_id", "mount_point", "mount_id", "f"."path", "mount_provider_class" FROM "oc_mounts" "m" INNER JOIN "oc_filecache" "f" ON "m"."root_id" = "f"."fileid" WHERE ("storage_id" = ?) AND ("user_id" = ?)
  + SELECT "filecache"."fileid", "storage", "path", "path_hash", "filecache"."parent", "filecache"."name", "mimetype", "mimepart", "size", "mtime", "storage_mtime", "encrypted", "etag", "permissions", "checksum", "unencrypted_size", "metadata_etag", "creation_time", "upload_time" FROM "oc_filecache" "filecache" LEFT JOIN "oc_filecache_extended" "fe" ON "filecache"."fileid" = "fe"."fileid" WHERE "filecache"."fileid" = :dcValue1
  + SELECT "storage_id", "root_id", "user_id", "mount_point", "mount_id", "f"."path", "mount_provider_class" FROM "oc_mounts" "m" INNER JOIN "oc_filecache" "f" ON "m"."root_id" = "f"."fileid" WHERE ("storage_id" = ?) AND ("user_id" = ?)
  + SELECT "filecache"."fileid", "storage", "path", "path_hash", "filecache"."parent", "filecache"."name", "mimetype", "mimepart", "size", "mtime", "storage_mtime", "encrypted", "etag", "permissions", "checksum", "unencrypted_size", "metadata_etag", "creation_time", "upload_time" FROM "oc_filecache" "filecache" LEFT JOIN "oc_filecache_extended" "fe" ON "filecache"."fileid" = "fe"."fileid" WHERE "filecache"."fileid" = :dcValue1
≠ /remote.php/dav/files/test/new_file.txt with 1 queries removed
  - DELETE FROM "oc_file_metadata" WHERE "id" = :dcValue1

@github-actions
Copy link
Contributor

Possible performance regression detected

Show Output
4 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 5 queries added
  + SELECT "storage", "path", "mimetype" FROM "oc_filecache" WHERE "fileid" = :dcValue1
  + SELECT "storage_id", "root_id", "user_id", "mount_point", "mount_id", "f"."path", "mount_provider_class" FROM "oc_mounts" "m" INNER JOIN "oc_filecache" "f" ON "m"."root_id" = "f"."fileid" WHERE ("storage_id" = ?) AND ("user_id" = ?)
  + SELECT "filecache"."fileid", "storage", "path", "path_hash", "filecache"."parent", "filecache"."name", "mimetype", "mimepart", "size", "mtime", "storage_mtime", "encrypted", "etag", "permissions", "checksum", "unencrypted_size", "metadata_etag", "creation_time", "upload_time" FROM "oc_filecache" "filecache" LEFT JOIN "oc_filecache_extended" "fe" ON "filecache"."fileid" = "fe"."fileid" WHERE "filecache"."fileid" = :dcValue1
  + SELECT "storage_id", "root_id", "user_id", "mount_point", "mount_id", "f"."path", "mount_provider_class" FROM "oc_mounts" "m" INNER JOIN "oc_filecache" "f" ON "m"."root_id" = "f"."fileid" WHERE ("storage_id" = ?) AND ("user_id" = ?)
  + SELECT "filecache"."fileid", "storage", "path", "path_hash", "filecache"."parent", "filecache"."name", "mimetype", "mimepart", "size", "mtime", "storage_mtime", "encrypted", "etag", "permissions", "checksum", "unencrypted_size", "metadata_etag", "creation_time", "upload_time" FROM "oc_filecache" "filecache" LEFT JOIN "oc_filecache_extended" "fe" ON "filecache"."fileid" = "fe"."fileid" WHERE "filecache"."fileid" = :dcValue1
≠ /remote.php/dav/files/test/new_file.txt with 1 queries removed
  - DELETE FROM "oc_file_metadata" WHERE "id" = :dcValue1

@artonge artonge force-pushed the artonge/fix/listen_to_cache_events_for_metadata branch from 11b56d0 to de007ae Compare October 19, 2022 09:09
@CarlSchwan
Copy link
Member

The performance bot is finding suspicious things

@github-actions
Copy link
Contributor

Possible performance regression detected

Show Output
4 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 5 queries added
  + SELECT "storage", "path", "mimetype" FROM "oc_filecache" WHERE "fileid" = :dcValue1
  + SELECT "storage_id", "root_id", "user_id", "mount_point", "mount_id", "f"."path", "mount_provider_class" FROM "oc_mounts" "m" INNER JOIN "oc_filecache" "f" ON "m"."root_id" = "f"."fileid" WHERE ("storage_id" = ?) AND ("user_id" = ?)
  + SELECT "filecache"."fileid", "storage", "path", "path_hash", "filecache"."parent", "filecache"."name", "mimetype", "mimepart", "size", "mtime", "storage_mtime", "encrypted", "etag", "permissions", "checksum", "unencrypted_size", "metadata_etag", "creation_time", "upload_time" FROM "oc_filecache" "filecache" LEFT JOIN "oc_filecache_extended" "fe" ON "filecache"."fileid" = "fe"."fileid" WHERE "filecache"."fileid" = :dcValue1
  + SELECT "storage_id", "root_id", "user_id", "mount_point", "mount_id", "f"."path", "mount_provider_class" FROM "oc_mounts" "m" INNER JOIN "oc_filecache" "f" ON "m"."root_id" = "f"."fileid" WHERE ("storage_id" = ?) AND ("user_id" = ?)
  + SELECT "filecache"."fileid", "storage", "path", "path_hash", "filecache"."parent", "filecache"."name", "mimetype", "mimepart", "size", "mtime", "storage_mtime", "encrypted", "etag", "permissions", "checksum", "unencrypted_size", "metadata_etag", "creation_time", "upload_time" FROM "oc_filecache" "filecache" LEFT JOIN "oc_filecache_extended" "fe" ON "filecache"."fileid" = "fe"."fileid" WHERE "filecache"."fileid" = :dcValue1
≠ /remote.php/dav/files/test/new_file.txt with 1 queries removed
  - DELETE FROM "oc_file_metadata" WHERE "id" = :dcValue1

@artonge artonge force-pushed the artonge/fix/listen_to_cache_events_for_metadata branch 2 times, most recently from 8af96af to 5a4502c Compare October 24, 2022 13:16
}

private function isCorrectPath(string $path): bool {
// TODO make this more dynamic, we have the same issue in other places
return !str_starts_with($path, 'appdata_') && !str_starts_with($path, 'files_versions/') && !str_starts_with($path, 'files_trashbin/');
return !str_starts_with($path, 'appdata_') && !str_starts_with($path, 'files_versions/');
Copy link
Contributor Author

@artonge artonge Oct 24, 2022

Choose a reason for hiding this comment

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

Review hint: Removed file_trashbin as we want to react on CacheEntryRemovedEvent even if the node is in the trash bin.

@PVince81 PVince81 requested a review from come-nc October 25, 2022 15:30
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

What is the difference between OCP\Files\Events\NodeRemovedFromCache and OCP\Files\Cache\CacheEntryRemovedEvent ?

@artonge
Copy link
Contributor Author

artonge commented Nov 3, 2022

What is the difference between OCP\Files\Events\NodeRemovedFromCache and OCP\Files\Cache\CacheEntryRemovedEvent ?

  • NodeRemovedFromCache is emitted from the Scanner.php
  • CacheEntryRemovedEvent is emitted from the Cache.php

Apart from that, I don't really know ^^

@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 modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge
Copy link
Contributor Author

artonge commented Nov 21, 2023

Superseded by #41634

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: oc_file_metadata preservation issues with trashbin
4 participants