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.

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

Use index to mark parent as outdated, fixes: #14537 #14571

Merged
merged 1 commit into from
Mar 19, 2019
Merged

Use index to mark parent as outdated, fixes: #14537 #14571

merged 1 commit into from
Mar 19, 2019

Conversation

ariselseng
Copy link
Member

@ariselseng ariselseng commented Mar 6, 2019 •
edited

Make inner join to make at least mariadb to use index in update query in Notify.php
Fixes #14537

@ariselseng ariselseng force-pushed the optimize-files_external-notify branch from d471d2d to 0f41197 Compare March 6, 2019 15:21
@ariselseng ariselseng requested a review from kesselb March 6, 2019 15:24
@ariselseng ariselseng added the 2. developing Work in progress label Mar 7, 2019
@ariselseng ariselseng removed the request for review from kesselb March 7, 2019 15:50
@ariselseng ariselseng force-pushed the optimize-files_external-notify branch from 0f41197 to 2ab0bb7 Compare March 7, 2019 17:18
@ariselseng ariselseng added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 7, 2019
@icewind1991
Copy link
Member

You're missing the comparison on mount_id.

Also it would be nice to move this over the using the query builder

@ariselseng
Copy link
Member Author

Wow that went a little to fast 😄

@ariselseng ariselseng force-pushed the optimize-files_external-notify branch from 2ab0bb7 to ef5ebd6 Compare March 7, 2019 17:48
@ariselseng
Copy link
Member Author

@icewind1991 Doesn't look like querybuilder works with UPDATE and INNER JOIN: https://stackoverflow.com/questions/15293502/doctrine-query-builder-not-working-with-update-and-inner-join

@ariselseng
Copy link
Member Author

@icewind1991 Actually, UPDATE with JOINS is not supported in sqlite. Cleanest way would probably be to do it in two queries then. What do you think?

@ariselseng ariselseng added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 7, 2019
@ariselseng ariselseng force-pushed the optimize-files_external-notify branch from ef5ebd6 to ab8b977 Compare March 8, 2019 08:08
@ariselseng
Copy link
Member Author

@icewind1991 Alright, now it uses two queries. I moved the reconnect logic to the storageIds query instead. I don't think we need both. I also couldn't figure out how reuse them as prepared statements. Is this okay?

@ariselseng ariselseng added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 8, 2019
@ariselseng ariselseng force-pushed the optimize-files_external-notify branch from ab8b977 to 835d023 Compare March 8, 2019 11:49
@ariselseng
Copy link
Member Author

@kesselb @icewind1991 How does it look now?

One thing in my case where a subfolder is not accessible/listable and hidden on the share for the SMB user. The notify changes are still listed by the SMB server even though the user listening do not have access (I think only NTFS permissions is set). Each time that subfolder is changed, this code will log about missing parent. Probably only a problem for a very few.

@ariselseng ariselseng force-pushed the optimize-files_external-notify branch from 835d023 to b7e2594 Compare March 8, 2019 13:13
@icewind1991
Copy link
Member

looks good otherwise

@ariselseng ariselseng force-pushed the optimize-files_external-notify branch 2 times, most recently from 37bbb34 to c0dc0ee Compare March 8, 2019 15:33
@ariselseng ariselseng force-pushed the optimize-files_external-notify branch from c0dc0ee to 4aa30cd Compare March 8, 2019 15:34
@ariselseng ariselseng mentioned this pull request Mar 12, 2019
Closed
@ariselseng
Copy link
Member Author

@rullzer @kesselb Any chance of a review? This should make it to 16. Thanks.

kesselb
kesselb reviewed Mar 15, 2019
View reviewed changes
apps/files_external/lib/Command/Notify.php Show resolved Hide resolved
apps/files_external/lib/Command/Notify.php Show resolved Hide resolved
apps/files_external/lib/Command/Notify.php Outdated Show resolved Hide resolved
apps/files_external/lib/Command/Notify.php Outdated
$storageIds = $this->getStorageIds($mountId);
}

if (!$storageIds || count($storageIds) === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect that getStorageIds always returns an array $storageIds === [] should work.

apps/files_external/lib/Command/Notify.php Outdated
} catch (DriverException $ex) {
$this->logger->logException($ex, ['app' => 'files_external', 'message' => 'Error while trying to mark folder as outdated', 'level' => ILogger::WARN]);
$this->logger->logException($ex, ['app' => 'files_external', 'message' => 'Error while trying to find correct storage ids.', 'level' => ILogger::WARN]);
Copy link
Contributor

Choose a reason for hiding this comment

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

The logger should set the right app here. Could you test it without 'app' => 'files_external'?

@ariselseng ariselseng force-pushed the optimize-files_external-notify branch from 4aa30cd to 575de52 Compare March 15, 2019 16:51
@ariselseng
Copy link
Member Author

@kesselb Thank you for you input. I have pushed changes now. Care to have a look again?

@ariselseng
Use two queries to mark parent as outdated
11b3fbf 
Signed-off-by: Ari Selseng <ari@selseng.net>
@ariselseng ariselseng force-pushed the optimize-files_external-notify branch from 575de52 to 11b3fbf Compare March 18, 2019 17:25
@ariselseng ariselseng requested a review from kesselb March 18, 2019 17:27
@kesselb
Copy link
Contributor

kesselb commented Mar 18, 2019

Don't have a working test setup for smb 😞 Code looks good 👍

@ariselseng
Copy link
Member Author

@MorrisJobke @rullzer is this mergeable?

MorrisJobke
MorrisJobke approved these changes Mar 19, 2019
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke 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 👍

@MorrisJobke MorrisJobke merged commit ff6f105 into nextcloud:master Mar 19, 2019
@MorrisJobke MorrisJobke added this to the Nextcloud 16 milestone Mar 19, 2019
@MorrisJobke MorrisJobke mentioned this pull request Mar 20, 2019
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@icewind1991 icewind1991 icewind1991 approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@kesselb kesselb Awaiting requested review from kesselb

Assignees
No one assigned
Labels
3. to review Waiting for reviews feature: external storage
Projects
None yet
Milestone
Nextcloud 16
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ariselseng @icewind1991 @kesselb @MorrisJobke