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

feat: remove orphaned entries from filecache_extended #38933

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Jun 21, 2023

  • Resolves: #

Summary

Test scenario:

  • Run sql: select * from oc_filecache_extended left join oc_filecache on oc_filecache_extended.fileid = oc_filecache.fileid where oc_filecache.fileid is null;
  • Mount an external storage (e.g. local)
  • Upload some files to the external storage
  • Delete external storage
  • Run sql: select * from oc_filecache_extended left join oc_filecache on oc_filecache_extended.fileid = oc_filecache.fileid where oc_filecache.fileid is null;
  • See files without a matching record in filecache

public static function cleanByMountId(int $mountId) {

Best would be to clear filecache_extended when deleting the storage.

TODO

  • OCI
  • CI

Checklist

@kesselb kesselb added this to the Nextcloud 28 milestone Jun 21, 2023
@kesselb kesselb self-assigned this Jun 21, 2023
@kesselb kesselb force-pushed the orphaned-entries-filecache-extended branch from 4c3a6c7 to c84aef5 Compare June 21, 2023 16:55
@kesselb kesselb requested review from juliusknorr, icewind1991, a team, ArtificialOwl and nfebe and removed request for a team June 21, 2023 16:55
@kesselb kesselb added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 21, 2023
@@ -48,7 +50,8 @@ public function __construct(IDBConnection $connection) {
protected function configure() {
$this
->setName('files:cleanup')
->setDescription('cleanup filecache');
->setDescription('cleanup filecache')
->addOption('filecache-extended', null, InputOption::VALUE_NONE, 'remove orphaned entries from filecache_extended');
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not make this run by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, no. The execution for the left join seems quick, but I only tested with ~1000 files.

My motivation, to hide it behind a flag, is my incomplete knowledge about filecache and especially external storages. However, I can't think of a case, why we would have a record in filecache_extended without a matching one in filecache.

We can run it by default if you think it should ;)

Copy link
Member

Choose a reason for hiding this comment

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

Switched to running it by default and having the flag disable it in case we ever find cases where that is required

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Code looks good, small question inside :)

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
@kesselb
Copy link
Contributor Author

kesselb commented Nov 14, 2023

Moving to 29

@kesselb kesselb modified the milestones: Nextcloud 28, Nextcloud 29 Nov 14, 2023
@icewind1991 icewind1991 force-pushed the orphaned-entries-filecache-extended branch from c84aef5 to da57476 Compare February 9, 2024 13:50
@kesselb kesselb force-pushed the orphaned-entries-filecache-extended branch from da57476 to 9fb2258 Compare February 17, 2024 16:14
@kesselb
Copy link
Contributor Author

kesselb commented Feb 17, 2024

Howdy 👋

Thank you for taking care, I almost forgot about it 🙈

I don't remember if I tested it with OCI back then.
The chunking (for the OCI in clauses limit) is there.

@skjnldsv skjnldsv force-pushed the orphaned-entries-filecache-extended branch from 9fb2258 to 3b73c89 Compare February 24, 2024 16:11
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 24, 2024
@skjnldsv
Copy link
Member

1) OCA\Files\Tests\Command\DeleteOrphanedFilesTest::testClearFiles
Expectation failed for method name is "writeln" when invoked 2 time(s)
Parameter 0 for invocation #1 Symfony\Component\Console\Output\OutputInterface::writeln('0 orphaned file cache extende...eleted', 0) does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1 orphaned mount entries deleted'
+'0 orphaned file cache extended entries deleted'

@skjnldsv skjnldsv added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Feb 24, 2024
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@kesselb kesselb force-pushed the orphaned-entries-filecache-extended branch 2 times, most recently from 63353ba to 0635c24 Compare March 26, 2024 10:46
@kesselb kesselb added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 26, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@kesselb
Copy link
Contributor Author

kesselb commented Mar 28, 2024

Moving to 30

@kesselb kesselb modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@kesselb kesselb force-pushed the orphaned-entries-filecache-extended branch from 0635c24 to 3e0299f Compare April 8, 2024 19:43
kesselb and others added 2 commits April 15, 2024 20:30
@kesselb kesselb force-pushed the orphaned-entries-filecache-extended branch from 3e0299f to 1d34f0a Compare April 15, 2024 18:30
@icewind1991 icewind1991 merged commit b5e3508 into master Apr 15, 2024
157 checks passed
@icewind1991 icewind1991 deleted the orphaned-entries-filecache-extended branch April 15, 2024 19:07
@blizzz blizzz mentioned this pull request Jul 24, 2024
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.

4 participants