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

Fix size propagation over shared storage boundary #14720

Merged
merged 2 commits into from
Mar 9, 2015

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Mar 5, 2015

Fix size propagation for #14596

  • propagate sizes over shared storage boundary
  • unit test

@icewind1991 FYI

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 5, 2015

@icewind1991 I wonder if at some point we should move size calculation to the propagator classes

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 5, 2015

It turns out size propagation is easy but etag/mtime propagation is hard.

The "Updater" class is based on the current user's view, so it's not possible to make it work on another user's view without setupFS gymnastics... I also tried to instantiate a ChangePropagator from the SharedScanner, but that's another code path and isn't triggered.

@icewind1991 any suggestion how to make this work ?
If it's too complicated I might split this into a separate PR so we can at least get the size stuff in...

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 5, 2015

Keep in mind that the fix needs to be backportable to OC 7...

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 6, 2015

Let's keep this here only for the size propagation.
Etag propagation is another story, to be looked into separately: #14726

@PVince81 PVince81 added this to the 8.1-current milestone Mar 6, 2015
@luizluca
Copy link

luizluca commented Mar 6, 2015

@PVince81 , I just tested and it works for me :-)
Thanks,

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 6, 2015

Great to hear that, thanks for testing.

I still need to write a unit test to finish this PR.

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 6, 2015

...that assuming that @icewind1991 is fine with the approach I used.

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 9, 2015

Just had a go at a unit test locally and the bad news is that it seems to propagate wrong.

Whenever a share recipient changes a file, the size should be propagated to the owner's root, NOT to the recipient's root. The propagation must stop at the shared folder and not go further. It doesn't seem to be the case at the moment. This means that the size would be counted twice, once for each user, which is wrong.

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 9, 2015

Hmmm, I tried this manually and the propagation seems to work properly. The recipient's root size stays the same. Maybe something is wrong in the unit test then.

@PVince81 PVince81 force-pushed the fix-shareetagpropagation branch from 19512ed to ec19d9c Compare March 9, 2015 11:57
@scrutinizer-notifier
Copy link

A new inspection was created.

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 9, 2015

Ok fixed the unit test.
The mistake is that I used getFileInfo($path) which automatically includes the size of mount points.
The proper call is getFileInfo($path, false) to get what's in the DB.

@PVince81 PVince81 changed the title [WIP] Fix size and etag propagation over shared storage boundary Fix size and etag propagation over shared storage boundary Mar 9, 2015
@PVince81 PVince81 changed the title Fix size and etag propagation over shared storage boundary Fix size propagation over shared storage boundary Mar 9, 2015
@PVince81
Copy link
Contributor Author

PVince81 commented Mar 9, 2015

Test steps are here: #14596 (comment)

@ghost
Copy link

ghost commented Mar 9, 2015

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

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 9, 2015

Please review @icewind1991 @DeepDiver1975 @MorrisJobke @nickvergessen @schiesbn

The patch was tested by the original reporter here #14720 (comment)

@icewind1991
Copy link
Contributor

Looks good 👍

@MorrisJobke
Copy link
Contributor

Tested & works:+1:

MorrisJobke added a commit that referenced this pull request Mar 9, 2015
Fix size propagation over shared storage boundary
@MorrisJobke MorrisJobke merged commit 94b7fa1 into master Mar 9, 2015
@MorrisJobke MorrisJobke deleted the fix-shareetagpropagation branch March 9, 2015 15:24
@PVince81
Copy link
Contributor Author

PVince81 commented Mar 9, 2015

@karlitschek backport to OC 7 and OC 8 ? (for 7.0.7 and 8.0.3)
Size propagation / quota calculation is broken since OC 7 with shares

@karlitschek
Copy link
Contributor

Please backport once 8.0.2 and 7.0.5 are done :-) 👍

@PVince81
Copy link
Contributor Author

Preparing backport PRs now.

@PVince81
Copy link
Contributor Author

stable8: #14860
stable7: #14861

@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 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.

6 participants