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

Move preview repair to query #23073

Closed

rullzer wants to merge 2 commits into master from enh/preview_repair_query

Member

rullzer commented Sep 28, 2020

The get directory contents can take ages on a large system. Better chunk
it up.

Signed-off-by: Roeland Jago Douma roeland@famdouma.nl


          Move preview repair to query

1bf3379

The get directory contents can take ages on a large system. Better chunk
it up.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer added enhancement 3. to review feature: previews and thumbnails labels

rullzer added this to the Nextcloud 21 milestone

rullzer requested review from nickvergessen, MorrisJobke, icewind1991, ChristophWurst and juliushaertl

September 28, 2020 12:38

Member

icewind1991 commented Sep 30, 2020

Do you have any data on how this affects the performance, while the "startup costs" should indeed be lower, I suspect that the extra overhead of getting each folder object by name will make things run slower in the end.

Member Author

rullzer commented Oct 1, 2020

Do you have any data on how this affects the performance, while the "startup costs" should indeed be lower, I suspect that the extra overhead of getting each folder object by name will make things run slower in the end.

Well we can also fetch a bit more.
The issue here is that for big systems (>150k users) the preview folder is so big that fetching it in one go basically does boom. Fetching it now in batches might have a total higher overhead but each query should be fast.

nickvergessen reviewed

View reviewed changes

core/Command/Preview/Repair.php

+              		$cursor = $qb->select($qb->func()->count('*'))
+              			->from('filecache')
+              			->where($qb->expr()->eq('parent', $qb->createNamedParameter($currentPreviewFolder->getId())))
+              			->orderBy('fileid', 'ASC')

Member

nickvergessen Oct 1, 2020

if you just count, don't order

MorrisJobke reviewed

View reviewed changes

core/Command/Preview/Repair.php

+              					$hasFiles = false;
+              					foreach ($listing as $node) {
+              						if (!($node instanceof Folder)) {
+              							$hasFiles = true;

Member

MorrisJobke Oct 5, 2020

Suggested change

      
            							$hasFiles = true;
          
            							$hasFiles = true;
          
            							continue;

MorrisJobke reviewed

View reviewed changes

core/Command/Preview/Repair.php

-              					$section1->writeln("         Move preview/$name/$previewName to preview/$newFoldername", OutputInterface::VERBOSITY_VERBOSE);
+              				}
+              				$hasEntries = true;

Member

MorrisJobke Oct 5, 2020

This only triggers the while loop to continue if there are folders in the current set, that are processed. But what if there are only skipped folders in the current set? Then it would never reach the end. I can't come up with anything that would hold 1000 non-numeric folders, so this might be a theoretical issue.

MorrisJobke reviewed

View reviewed changes

core/Command/Preview/Repair.php

+              		do {
+              			$qb = $this->db->getQueryBuilder();
+              			$cursor = $qb->select('name')->from('filecache')
+              				->where($qb->expr()->eq('parent', $qb->createNamedParameter($currentPreviewFolder->getId())))

Member

MorrisJobke Oct 5, 2020

Wouldn't it make sense to remember the highest fileID from the previous loop run and put it in here as well?

Suggested change

      
            				->where($qb->expr()->eq('parent', $qb->createNamedParameter($currentPreviewFolder->getId())))
          
            				->where($qb->expr()->eq('parent', $qb->createNamedParameter($currentPreviewFolder->getId())))
          
            				->andWhere($qb->expr()->gt('fileid', $qb->createNamedParameter($previousHighestFileId)))

MorrisJobke reviewed

View reviewed changes

core/Command/Preview/Repair.php Outdated

-              			$newFoldername = Root::getInternalFolder($name);
+              			$hasEntries = false;
+              			while ($row = $cursor->fetch()) {
+              				$oldPreviewFolder = $currentPreviewFolder->get($row['name']);

Member

MorrisJobke Oct 5, 2020

Suggested change

      
            				$oldPreviewFolder = $currentPreviewFolder->get($row['name']);
          
            				$oldPreviewFolder = $currentPreviewFolder->get($row['name']);
          
            				$previousHighestFileId = $oldPreviewFolder->getId();

MorrisJobke reviewed

View reviewed changes

core/Command/Preview/Repair.php

-              				$progressBar->advance();
-              				continue;
-              			}
+              		$hasEntries = true;

Member

MorrisJobke Oct 5, 2020

Suggested change

      
            		$hasEntries = true;
          
            		$hasEntries = true;
          
            		$previousHighestFileId = 0;


          Ignore NotFoundException when fetching the old folder

656e58a

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

faily-bot bot commented Oct 12, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 34040: failure

mariadb10.1-php7.3

I'm a little sad 🤖 and was not able to find the logs for this failed job - please improve me at https://github.com/MorrisJobke/drone-logs to provide this to you

mysql8.0-php7.4

cancelled - typically means that the tests took longer than the drone CI allows them to run

acceptance-app-files-sharing-link

tests/acceptance/features/app-files-sharing-link.feature:26

Show full log

  Scenario: show download again in a public shared link             # /drone/src/tests/acceptance/features/app-files-sharing-link.feature:26
    Given I act as John                                             # ActorContext::iActAs()
    And I am logged in                                              # LoginPageContext::iAmLoggedIn()
    And I share the link for "welcome.txt"                          # FilesAppSharingContext::iShareTheLinkFor()
    And I set the download of the shared link as hidden             # FilesAppSharingContext::iSetTheDownloadOfTheSharedLinkAsHidden()
      Share link menu trigger in the details view in Files app could not be found after 20 seconds (NoSuchElementException)
    And I set the download of the shared link as shown              # FilesAppSharingContext::iSetTheDownloadOfTheSharedLinkAsShown()
    And I write down the shared link                                # FilesAppSharingContext::iWriteDownTheSharedLink()
    When I act as Jane                                              # ActorContext::iActAs()
    And I visit the shared link I wrote down                        # PublicShareContext::iVisitTheSharedLinkIWroteDown()
    And I see that the current page is the shared link I wrote down # PublicShareContext::iSeeThatTheCurrentPageIsTheSharedLinkIWroteDown()
    Then I see that the download button is shown                    # PublicShareContext::iSeeThatTheDownloadButtonIsShown()
    And I open the Share menu                                       # PublicShareContext::iOpenTheShareMenu()
    And I see that the Share menu is shown                          # PublicShareContext::iSeeThatTheShareMenuIsShown()

skjnldsv added the performance 🚀 label

This was referenced Dec 14, 2020

21 beta2 #24685

Merged

21 beta3 #24765

Merged

This was referenced Dec 28, 2020

21 beta4 #24852

Merged

21 beta5 #24951

Merged

rullzer modified the milestones: Nextcloud 21, Nextcloud 22

Member Author

rullzer commented Mar 30, 2021

I'm going to close this due to lack of activity.
Feel free to reopen if anybody wants to continue.

rullzer closed this

rullzer deleted the enh/preview_repair_query branch

March 30, 2021 19:43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

nickvergessen nickvergessen left review comments

MorrisJobke MorrisJobke left review comments

icewind1991 Awaiting requested review from icewind1991

ChristophWurst Awaiting requested review from ChristophWurst

juliushaertl Awaiting requested review from juliushaertl