Large federated shares #28059

butonic · 2017-06-01T10:38:10Z

Prevent timeouts when adding large federated shares:

completely removes getShareinfo() and instead uses PROPFIND
removes the Scanner, as it is no longer needed
shareinfo.php is left in because it may be used by existing instances
- a deprecation message is added to the JSON response
- should we add sth like will be removed in OC 11?
removes external.php because it predates the OCS api, but we might want to only log deprecation warnings, as I did for shareinfo.php

backport needed to 9.1

PVince81 · 2017-06-01T11:21:16Z

apps/files_sharing/ajax/shareinfo.php

+
 OCP\JSON::checkAppEnabled('files_sharing');

+OC::$server->getLogger()->warning(


Not sure if we can afford this... likely to flood logs. Switch to debug maybe ?

Also an admin might not know what hundreds of different remotes are connecting to this instance as this is controlled by the users.

My intention is to annoy admins to get them to upgrade older versions. And if they have to annoy another admin I am fine with it. Saves everyone problems and time in the long run.

following MS' footsteps with Win10 upgrades ? not sure...

I strongly vote for reverting those 4 lines (34-37). It´s annoying and - guess what - really helps NOTHING, especially when all the remote servers are hosted instances without admin access or even when they run Nextcloud.

PVince81 · 2017-06-01T11:25:38Z

should we add sth like will be removed in OC 11?

not really as this was never officially public API...

Code changed look good, needs good testing, especially the "detect temporarily unavailable remote server" cases.

TEST: accepting share without notification app enabled (just in case because of external.php)
TEST: accepting share with notification app enabled (just in case because of external.php)
TEST: accessing/syncing federated share
TEST: mounting link share as federated share
TEST: sharer removes share access, StorageInvalidException, receiver's mount disappears at first access
TEST: sharer changes password, StorageInvalidException, receiver's mount disappears at first access
TEST: sharer OC in maintenance mode, StorageNotAvailableException, receiver's mount stays and returns 503. When problem resolved, share is accessible again.
TEST: sharer OC returns 404 (move away all code), StorageNotAvailableException, receiver's mount stays and returns 503. When problem resolved, share is accessible again. => seems it still lets me browse the folders, but was already the case on master... so counting as "no regression here"

PVince81 · 2017-06-01T11:48:39Z

possible extension: make the DAV storage read oc:size from PROPFIND and return it (if the scanner does support reading this value), something for another PR

PVince81 · 2017-06-01T12:04:06Z

@butonic sadly it looks like we'll have to keep "ajax/external.php", it's used when mounting link shares... but will need to replace its "scanAll" call with something that only scans one level / the root entry.

PVince81 · 2017-06-01T12:12:02Z

local testing went fine except for the public link shares

PVince81 · 2017-07-04T09:55:15Z

@tomneedham can you help finishing this ?

PVince81 · 2017-07-07T06:59:57Z

Pushed changes to bring back ajax/external.php but without using the removed scanAll.

Had a quick look at using oc:size but it will be a little bit more work: need the DAV class to override Common::getMetaData() because that method always uses -1 for folders.

PVince81 · 2017-08-02T15:13:04Z

This is ready for re-review, manual tests passed.

I hereby approve of @butonic's commits, given that they require my additions.

@butonic please review my additional commits.

butonic · 2017-08-03T13:52:36Z

@PVince81 changes make sense

core/apps/files_sharing/appinfo/routes.php

Lines 64 to 65 in f10d105

    
           $this->create('sharing_external_add', '/external') 
        
           	->actionInclude('files_sharing/ajax/external.php');

is the place where we use ajax/external.php and make it available as /external

https://github.com/owncloud/core/blob/master/apps/files_sharing/js/external.js#L103 is the place where we POST to the url

PVince81 · 2017-08-03T14:14:33Z

@butonic is that a thumbs up for merging or do you think we need further changes ?

tomneedham · 2017-08-03T14:31:35Z

Tested with two instances running this. Remote sharing a folder worked, accessed the files, and uploaded between the two instances. 👍

PVince81 · 2017-08-03T14:35:51Z

let me rebase this for CI...

Backport to stable10 only or further ?

butonic · 2017-08-04T14:35:02Z

@PVince81 changes make sense

core/apps/files_sharing/appinfo/routes.php

Lines 64 to 65 in f10d105

    
           $this->create('sharing_external_add', '/external') 
        
           	->actionInclude('files_sharing/ajax/external.php');

is the place where we use ajax/external.php and make it available as /external

https://github.com/owncloud/core/blob/master/apps/files_sharing/js/external.js#L103 is the place where we POST to the url

PVince81 · 2017-08-07T08:20:57Z

@jfd let's do the route change separately as it's not related to the performance issue we want to solve and backport

PVince81 · 2017-08-07T14:03:35Z

stable10: #28604

@butonic should we backport further ?

butonic · 2017-08-08T11:35:27Z

I prefer getting 10.0.3 ready over a backport to 9.1. @felixboehm ?

lock · 2019-08-02T05:02:26Z

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.

butonic added feature:federated-cloud-sharing feature:sharing p1-urgent Critical issue, need to consider hotfix with just that issue performance sev1-critical labels Jun 1, 2017

butonic added this to the 10.0.3 milestone Jun 1, 2017

butonic requested review from DeepDiver1975 and PVince81 June 1, 2017 10:38

PVince81 reviewed Jun 1, 2017

View reviewed changes

PVince81 added the 2 - Developing label Jun 29, 2017

PVince81 assigned butonic Jul 11, 2017

PVince81 added 3 - To Review and removed 2 - Developing labels Aug 2, 2017

butonic and others added 5 commits August 3, 2017 16:36

get rid of External Scanner

6add3d3

remove support for non OCS endpoint to add remote shares

5aaed29

deprecate shareapi.php

6b73884

Bring back ajax/external.php, sadly

c138ee2

Remove scanAll from ajax/external

bb71b69

PVince81 force-pushed the large-federated-shares branch from 012fc3d to bb71b69 Compare August 3, 2017 14:36

PVince81 merged commit 0fb4345 into master Aug 7, 2017

PVince81 deleted the large-federated-shares branch August 7, 2017 14:00

PVince81 mentioned this pull request Aug 7, 2017

[stable10] Large federated shares #28604

Merged

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019


		OCP\JSON::checkAppEnabled('files_sharing');

		OC::$server->getLogger()->warning(

Large federated shares #28059

Large federated shares #28059

Uh oh!

Conversation

butonic commented Jun 1, 2017

Uh oh!

PVince81 Jun 1, 2017

Choose a reason for hiding this comment

Uh oh!

butonic Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

bcutter Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Jun 1, 2017

Uh oh!

PVince81 commented Jun 1, 2017

Uh oh!

PVince81 commented Jun 1, 2017

Uh oh!

PVince81 commented Jul 4, 2017

Uh oh!

PVince81 commented Jul 7, 2017

Uh oh!

PVince81 commented Aug 2, 2017

Uh oh!

butonic commented Aug 3, 2017

Uh oh!

PVince81 commented Aug 3, 2017

Uh oh!

tomneedham commented Aug 3, 2017

Uh oh!

PVince81 commented Aug 3, 2017

Uh oh!

butonic commented Aug 4, 2017

Uh oh!

PVince81 commented Aug 7, 2017

Uh oh!

PVince81 commented Aug 7, 2017

Uh oh!

butonic commented Aug 8, 2017

Uh oh!

lock bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PVince81 commented Jun 1, 2017 •

edited

Loading