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

Move preview repair to query #23073

Closed
wants to merge 2 commits into from
Closed

Move preview repair to query #23073

wants to merge 2 commits into from

Conversation

rullzer
Copy link
Member

@rullzer 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

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

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@icewind1991
Copy link
Member

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.

@rullzer
Copy link
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.

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

Choose a reason for hiding this comment

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

if you just count, don't order

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

Choose a reason for hiding this comment

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

Suggested change
$hasFiles = true;
$hasFiles = true;
continue;

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

$hasEntries = true;
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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)))

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

Choose a reason for hiding this comment

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

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

$progressBar->advance();
continue;
}
$hasEntries = true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$hasEntries = true;
$hasEntries = true;
$previousHighestFileId = 0;

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@faily-bot
Copy link

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

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()

This was referenced Dec 14, 2020
This was referenced Dec 28, 2020
@rullzer rullzer modified the milestones: Nextcloud 21, Nextcloud 22 Jan 6, 2021
@rullzer
Copy link
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 rullzer closed this Mar 30, 2021
@rullzer 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants