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

Query the cache when checking if a node exists #22506

Merged
merged 5 commits into from Mar 23, 2016

Conversation

icewind1991
Copy link
Contributor

Improves performance when working with slow storage backends

cc @PVince81 @rullzer

@icewind1991 icewind1991 added this to the 9.0-current milestone Feb 18, 2016
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @MorrisJobke, @PVince81 and @nickvergessen to be potential reviewers

@rullzer
Copy link
Contributor

rullzer commented Feb 19, 2016

Failing unit tests

@icewind1991 icewind1991 force-pushed the node-get-from-cache branch 3 times, most recently from 0f3fda1 to d1e747b Compare February 19, 2016 13:29
@icewind1991
Copy link
Contributor Author

Failing unit tests

All fixed

@@ -129,7 +129,12 @@ public static function initPaths() {
} else {
self::$configDir = OC::$SERVERROOT . '/config/';
}
self::$config = new \OC\Config(self::$configDir);

if (defined('PHPUNIT_RUN') and PHPUNIT_RUN) {
Copy link
Contributor

Choose a reason for hiding this comment

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

no way!

@nickvergessen
Copy link
Contributor

👎 for the hack in base.php

@icewind1991
Copy link
Contributor Author

Switched to mocking the config

I would still like to add a system to automatically handle it for all tests in the future

@icewind1991
Copy link
Contributor Author

@nickvergessen @rullzer @PVince81 please review

@rullzer
Copy link
Contributor

rullzer commented Feb 25, 2016

👍

@MorrisJobke
Copy link
Contributor

So this should really go into 9.0?

cc @karlitschek @cmonteroluque

@karlitschek
Copy link
Contributor

Hmmm. relatively risky after feature freeze and 3 days before the freeze. What do you think @cmonteroluque ?

@PVince81
Copy link
Contributor

This did not help fixing the original issue anyway, so I'd say 9.0.1

@PVince81
Copy link
Contributor

Moving to 9.0.1 as this doesn't fix any critical known issue

CC @cmonteroluque

@PVince81 PVince81 modified the milestones: 9.0.1-next-maintenance, 9.0-current Feb 29, 2016
@ghost
Copy link

ghost commented Mar 3, 2016

agreed

@@ -100,9 +100,9 @@ protected function createNode($path, $info = array()) {
$isDir = $info['mimetype'] === 'httpd/unix-directory';
}
if ($isDir) {
return new Folder($this->root, $this->view, $path);
return new Folder($this->root, $this->view, $path, $info);
Copy link
Member

Choose a reason for hiding this comment

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

$info seems to be an array as per PHPDoc. Folder seems however to expect \OCP\Files\FileInfo. Could you adjust this so that static code analysers are happy?

@MorrisJobke
Copy link
Contributor

@icewind1991 Could you checkout the above comments?

@icewind1991
Copy link
Contributor Author

switched everything to FileInfo objects

@icewind1991
Copy link
Contributor Author

@LukasReschke @MorrisJobke @PVince81 please review

@PVince81
Copy link
Contributor

👍

@PVince81
Copy link
Contributor

@nickvergessen @rullzer rereview after the changes ?

@MorrisJobke
Copy link
Contributor

Is the 9.0.1 milestone indented? I would rather not backport this and only merge it to master.

@rullzer rullzer modified the milestones: 9.1-current, 9.0.1-current-maintenance Mar 23, 2016
@rullzer
Copy link
Contributor

rullzer commented Mar 23, 2016

PR is against mater so goes into 9.1 anyway...

@MorrisJobke
Copy link
Contributor

Tested and works 👍

@DeepDiver1975 DeepDiver1975 merged commit ea07a42 into master Mar 23, 2016
@DeepDiver1975 DeepDiver1975 deleted the node-get-from-cache branch March 23, 2016 12:08
@icewind1991
Copy link
Contributor Author

#22463 is set for 9.0.1 which this is supposed to fix, should this be backported ( @karlitschek )

@PVince81
Copy link
Contributor

@icewind1991 this PR doesn't fix the underlying problem, so for now I'd rather say no backport.

@icewind1991
Copy link
Contributor Author

There are 2 parts of #22463

  • node api using the storage directly where it can use the cache which makes it slow on external storages
    • tested this using an artificial slowdown on storages
    • fixed with this pr
  • repeated scanning of storages making everything slow
    • haven't found any reliable way to reproduce this and I don't know the cause

@PVince81
Copy link
Contributor

Ok fair enough.

@karlitschek backport to 9.0.1 ?

@karlitschek
Copy link
Contributor

great. please backport

@icewind1991
Copy link
Contributor Author

stable9: #23520

@lock
Copy link

lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants