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

Sync Protocol: use oc:size instead of d:quota-used-bytes when browsing folders #4459

Closed
moscicki opened this issue Feb 10, 2016 · 13 comments
Closed
Assignees
Labels
Discussion ReadyToTest QA, please validate the fix/enhancement
Milestone

Comments

@moscicki
Copy link
Contributor

With 2.1 client when the folders are browsed (selective sync) the underlying PROPFIND queries for d:quota-used-bytes (ref: https://github.com/cernbox/smashbox/blob/master/protocol/protocol.md#quota-check-call)

It should not do so because:

  1. another property is already clearly defined for this purpose: oc:size
  2. the meaning of d:quota-used-bytes is not generally well defined for a directory when files of different file owners are in the tree

So for a storage system d:quota-used-bytes this is a property of the account (or quota node) but not that of the individual directory.

In the future I would consider to only use d:quota-used-bytes if you really want to know how much of his quota the user is using (as opposed to checking how big is the directory tree -- aka recursive accounting).

This is not a show-stopper for us currently because we can change the meaning of d:quota-used-bytes on the server but long-term this is not a good idea.

@guruz
Copy link
Contributor

guruz commented Feb 11, 2016

I remember @icewind1991 @PVince81 @ogoffart discussing about this..

@ogoffart
Copy link
Contributor

I did not know about oc:size. It's clearly what we should be using i guess.

@ogoffart ogoffart self-assigned this Feb 11, 2016
@PVince81
Copy link
Contributor

From what I see in core, "oc:size" calls getSize() on the Directory Sabre Node, which would return the value of the "size" column in oc_filecache. See https://github.com/owncloud/core/blob/v8.2.2/lib/private/connector/sabre/node.php#L190

The "quota-used-bytes" will retrieve the storage's used bytes, regardless which path you are querying.
However, there might be a difference if the path in question is on another external storage, then you'd get the used bytes on that specific storage. See https://github.com/owncloud/core/blob/v8.2.2/lib/private/connector/sabre/directory.php#L265

@PVince81
Copy link
Contributor

One more thing: the "size" column in oc_filecache is the sum of the size of all contents inside the queried folder, and this for the current storage. If there are mount points inside that folder (received shares, external storages), they are not counted in the size.

@ogoffart
Copy link
Contributor

Yes, we want the size. As in "How much harddrive space would it take if we sync this folder"

@moscicki
Copy link
Contributor Author

I propose to first settle on the protocol and then map it to the server implementation. My proposal is that
oc:size is used to discover the space occupied by the folder (a maximum space required to synchronize the data [maximum = taking into account that maybe some parts of the tree may not be accessed due to permissions]) and I would leave it up to the server internally to handle the case of remotely mounted storage (via whatever internal method it likes, caching etc).

Alternative is to make it non-transparent to the client such that the client is capable of finding out that the folder is an external storage mount and issue an extra request to ask some other property. But this is just shifting the job from the server to the client and complicating the whole protocol so maybe not the best idea.

Currently (2.1) the system is not consistent: it seems that quota-used-bytes is queried when user browses folders and oc:size is asked for in PROPFIND requests when discovering changes on the folders.

Please also, refer to this documentation: https://github.com/cernbox/smashbox/blob/master/protocol/protocol.md and this issue: #4455

@PVince81: your statement "The "quota-used-bytes" will retrieve the storage's used bytes, regardless which path you are querying." is currently true for our storage and the result looks like this:

image

So how comes it works for the owncloud server if your statement was true?

@icewind1991
Copy link

If there are mount points inside that folder (received shares, external storages), they are not counted in the size.

Incorrect, FileInfo->getSize takes submounts into account correctly

@icewind1991
Copy link

oc:size is used to discover the space occupied by the folder

That is currently the case

@PVince81
Copy link
Contributor

@icewind1991 ok, thanks for the clarification. Was this always the case ?

@icewind1991
Copy link

Yes

@ogoffart
Copy link
Contributor

Pushed in master for 2.2

@ogoffart ogoffart added this to the 2.2-next milestone Feb 11, 2016
@ogoffart ogoffart added the ReadyToTest QA, please validate the fix/enhancement label Feb 11, 2016
@moscicki
Copy link
Contributor Author

Thx!

I also updated the documentation accordingly (I hope this is compatible with current owncloud server implementation): cernbox/smashbox@ef70cbf

You may also want to think if you want to treat the external mount point in the owncloud server as a different quota node. I would say that you need to, because if a user had 1GB on the primary storage and 10G on the external mount, that 10GB free space is only available within that mountpoint (so on specific path). So if sync client wants to make a reasonable query about how much free space is left in the given directory before uploading to it, it should query that directory for available quota. Of course it can take shortcuts and understand that the quota information is valid for the entire directory tree associated with the quota node and not query all directories all the time. But for this you need to be able to discover mountpoints (i.e. different quota nodes).

This also brings to the next issue, which is that quota check should be done (and reported) on per sync-folder pair: imagine a user having two folder pairs: Documents (on primary storage) and Extended (mount on S3 storage). These are effectively two different quota nodes: there are two different quotas for these two different folders. It goes along similar lines as #4460

@ckamm
Copy link
Contributor

ckamm commented Mar 3, 2016

I'm closing this after looking at the patch again and checking that size information on folders still works.

@ckamm ckamm closed this as completed Mar 3, 2016
moscicki referenced this issue in owncloud/ios-library Mar 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion ReadyToTest QA, please validate the fix/enhancement
Projects
None yet
Development

No branches or pull requests

6 participants