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

[sharing] fix performance issues #11380

Merged
merged 2 commits into from Oct 10, 2014

Conversation

@schiessle
Copy link
Member

commented Oct 1, 2014

Fix performance issue when the user has folders with many files and want to share a new one.

#10588

@ghost

This comment has been minimized.

Copy link

commented Oct 2, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser/7678/

@LukasReschke LukasReschke changed the title [sharing] fix performance issues [WIP] [sharing] fix performance issues Oct 4, 2014
@ghost

This comment has been minimized.

Copy link

commented Oct 6, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng/6/

@schiessle schiessle force-pushed the fix_performance_issue_sharing branch 2 times, most recently to 41592c2 Oct 9, 2014
@schiessle schiessle changed the title [WIP] [sharing] fix performance issues [sharing] fix performance issues Oct 9, 2014
@schiessle

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2014

@owncloud-bot retest this please

1 similar comment
@PVince81

This comment has been minimized.

Copy link
Member

commented Oct 9, 2014

@owncloud-bot retest this please

@PVince81

This comment has been minimized.

Copy link
Member

commented Oct 9, 2014

@schiesbn if this is ready for review we can have a guided code review tomorrow 😄

@schiessle

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2014

yes, let's do this tomorrow

@schiessle

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2014

@owncloud-bot retest this please

* @param string $shareWith with whom should the item be shared
* @return array with shares
*/
public function getParents($itemSource, $shareWith = null) {

This comment has been minimized.

Copy link
@PVince81

PVince81 Oct 10, 2014

Member

Please add a unit test for this

$result['expirationDate'] = min($expirationDate, $checkReshare['expiration']);
if ($checkReshare[$column] === $itemSource) {

This comment has been minimized.

Copy link
@PVince81

PVince81 Oct 10, 2014

Member

Please add a comment to explain this better

@PVince81

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

To test:

  • reshare same file/folder
  • reshare file inside shared folder
  • reshare file inside shared folder, then unshare folder
  • reshare when another folder was shared with current user
  • test with #11510
  • link shares
  • link reshare
  • test with group share
  • group share + unshare from self
  • group reshare
@ghost

This comment has been minimized.

Copy link

commented Oct 10, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng/253/

Build result: FAILURE

[...truncated 7 lines...] > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/11380/merge^{commit} # timeout=10Checking out Revision 4784fa3cb0e4c043cc294dde0fd5aa0f83f190e8 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 4784fa3cb0e4c043cc294dde0fd5aa0f83f190e8 > git rev-list f53496f8c74a5de89f1082eba15c77d988670ef3 # timeout=10 > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng » sqlite,vm-slave-02Configuration pull-request-analyser-ng » sqlite,vm-slave-02 is still in the queue: Waiting for next available executor on vm-slave-02Triggering pull-request-analyser-ng » pgsql,vm-slave-02Triggering pull-request-analyser-ng » mysql,vm-slave-02Triggering pull-request-analyser-ng » oci,vm-slave-02Configuration pull-request-analyser-ng » pgsql,vm-slave-02 is still in the queue: Waiting for next available executor on vm-slave-02pull-request-analyser-ng » pgsql,vm-slave-02 completed with result FAILUREpull-request-analyser-ng » mysql,vm-slave-02 completed with result FAILUREpull-request-analyser-ng » oci,vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 2 second💣 Test FAILed. 💣

@schiessle schiessle force-pushed the fix_performance_issue_sharing branch from 41592c2 Oct 10, 2014
@schiessle

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2014

@PVince81 please try again, problem should be solved. I also added a unit test and a additional comment to explain the if statement

@PVince81

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

Fixes #11510

@PVince81

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

👍 tested, works well. Also did a bit of regression testing for sharing and didn't find any issues.

Needs a second reviewer @jnfrmarks @LukasReschke @MorrisJobke

@LukasReschke
LukasReschke reviewed Oct 10, 2014
View changes
apps/files_sharing/lib/share/file.php Outdated
@@ -49,6 +49,9 @@ public function getFilePath($itemSource, $uidOwner) {
$path = $this->path;
$this->path = null;
return $path;
} else {
$path = \OC\Files\Filesystem::getPath($itemSource);
return $path;
}
return false;

This comment has been minimized.

Copy link
@LukasReschke

LukasReschke Oct 10, 2014

Member

As far I can see the false will never be returned here - if so, then why not remove it?

This comment has been minimized.

Copy link
@LukasReschke

LukasReschke Oct 10, 2014

Member

Also the PHPDoc of the interface is stating that this function can return false which is wrong now.

@LukasReschke

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

👍 if my comments have been addressed.

@schiessle schiessle force-pushed the fix_performance_issue_sharing branch to 527e1d0 Oct 10, 2014
@scolebrook scolebrook force-pushed the master branch from 4a4ea1d to 9dea79e Oct 10, 2014
@ghost

This comment has been minimized.

Copy link

commented Oct 10, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng/284/

Build result: FAILURE

[...truncated 7 lines...] > git fetch --tags --progress https://github.com/owncloud/core.git +refs/heads/:refs/remotes/origin/ > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10 > git config remote.origin.fetch +refs/heads/:refs/remotes/origin/ # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Fetching upstream changes from https://github.com/owncloud/core.git > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/11380/merge^{commit} # timeout=10Checking out Revision b479c29b4aba8703f1e582de5623dd3e9fddd10a (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f b479c29b4aba8703f1e582de5623dd3e9fddd10a > git rev-list a704be688799333259a1efba74a82a5edab0dad5 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng » sqlite,vm-slave-02Configuration pull-request-analyser-ng » sqlite,vm-slave-02 is still in the queue: Waiting for next available executor on vm-slave-02Touchstone configurations resulted in FAILURE, so aborting...Started calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 1 second💣 Test FAILed. 💣

@schiessle

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2014

Jenkins seems to be broken, but tests passed locally.

schiessle added a commit that referenced this pull request Oct 10, 2014
@schiessle schiessle merged commit 87899db into master Oct 10, 2014
1 check failed
1 check failed
default Merged build finished.
Details
@schiessle schiessle deleted the fix_performance_issue_sharing branch Oct 10, 2014
@schiessle

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2014

@ghost

This comment has been minimized.

Copy link

commented Oct 10, 2014

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng/287/

Build result: FAILURE

💣 Test FAILed. 💣

@scrutinizer-notifier

This comment has been minimized.

Copy link

commented Oct 10, 2014

The inspection completed: 10 new issues, 9 updated code elements

@blizzz

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2014

It appears that this one broke Files view on master?
When i check out 87899db Merge pull request #11380 from owncloud/fix_performance_issue_sharing Files is empty, however
it is working with 527e1d0 try to get path from filesystem
there is nothing in between.

@schiessle

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2014

527e1d0 is part of this pull request. Not sure what the difference should be.

BTW, you also need this PR: #11542, this fixes a regression from this PR. (but this affects only unit tests and non-file shares)

Also master works just fine for me... If you still have problems on master please open a issue with more information.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.