Implementation of delta-sync support on server-side. #29404
Conversation
ahmedammar
force-pushed
the
master
branch
2 times, most recently
from
db9ebc6
to
f304357
Compare
ahmedammar
force-pushed
the
master
branch
6 times, most recently
from
e31d52b
to
3f49737
Compare
Codecov Report
@@ Coverage Diff @@
## master #29404 +/- ##
============================================
+ Coverage 58.24% 58.41% +0.17%
- Complexity 18548 18647 +99
============================================
Files 1092 1096 +4
Lines 63729 64012 +283
============================================
+ Hits 37117 37391 +274
- Misses 26612 26621 +9
Continue to review full report at Codecov.
|
apps/files/lib/Hooks.php
Outdated
|
|
||
| public static function connectHooks() { | ||
| \OCP\Util::connectHook('\OCP\Config', 'js', '\OCA\Files\App', 'extendJsConfig'); | ||
| \OCP\Util::connectHook('OC_Filesystem', 'post_rename', 'OCA\Files\Hooks', 'zsync_rename_hook'); |
There was a problem hiding this comment.
I don't really like this in here - we want to move the files app more and more into the direction of being a pure frontend app. We need this living somewhere in core.
@PVince81 or shall we move this zsync feature into it's own app?
There was a problem hiding this comment.
I can do whatever you guys decide, I just put it all in files app because it made sense to me.
There was a problem hiding this comment.
@DeepDiver1975 should we move this to the "dav" app then ?
| use OCP\AppFramework\Controller; | ||
| use \Exception; | ||
|
|
||
| class ZsyncApiController extends Controller { |
There was a problem hiding this comment.
where is this controller used?
There was a problem hiding this comment.
There was a problem hiding this comment.
I got that - but who is calling this? desktop client?
There was a problem hiding this comment.
Heh, yeah I thought you didn't need me to tell you that, yes used by client:
owncloud/client@2f4ba78#diff-81e8ad3e8074c317de5166e5be577fc6L451
There was a problem hiding this comment.
okay - can we move this to dav? We shall only use dav as api between clients and server
ahmedammar
force-pushed
the
master
branch
12 times, most recently
from
5988f3f
to
c1db4d2
Compare
apps/files/lib/Hooks.php
Outdated
| } | ||
|
|
||
| private static function zsync_copy_rename_get_paths( $params ) { | ||
| $from = $params[\OC\Files\Filesystem::signal_param_oldpath]; |
There was a problem hiding this comment.
As discussed with @ahmedammar on IRC, let's use file ids as the don't change on rename/move.
So the meta files are stored as "files_zsync/$fileid" then
|
Summary of discussion on IRC for the next steps, quoting @ahmedammar: |
|
Pending decisions for further steps:
|
ahmedammar
force-pushed
the
master
branch
2 times, most recently
from
da3a039
to
4701349
Compare
| Util::connectHook('OC_Filesystem', | ||
| 'write', | ||
| $this, | ||
| 'deleteZsyncMetadata'); |
There was a problem hiding this comment.
wait wait.. ok this PR needs hard performance review...
| * @param RequestInterface $request | ||
| * @param ResponseInterface $response | ||
| */ | ||
| function httpDelete(RequestInterface $request, ResponseInterface $response) { |
There was a problem hiding this comment.
I would love to see at some point some relations here.. without integrity contraints it can be a mess and corruption on corruption. I need to test what happens when I terminate script in very bad moment.
| } | ||
|
|
||
| // Disable if external storage used. | ||
| if (strpos($node->getDavPermissions(), 'M') === false) { |
There was a problem hiding this comment.
@DeepDiver1975 @butonic does this prevents also files_primary?
|
Apart from what @orzeech said, which if less usual case. For me this has to pass this test plan: DSC - delta sync client
DSC uploads new - > NC downloads -> NC uploads new
DSC1 uploads new -> DSC2 down&moves -> DSC1 downloads -> DSC1 uploads new
https://github.com/owncloud/smashbox/blob/master/lib/test_basicSync.py
I am bit worried on the amount of additional metadata (and data) operations it does. While for user it might not that much matter, it does for admins which pay for infrastructure. Fortunetelly this is mostly big files related (client side 10MB?) and as we saw big files are small number of all uploads/downloads. |
|
For the moment I have to give a red flag for this PR 👎 All the integration tests failed - for many reasons - please find my full delta sync report here https://cloud.owncloud.com/index.php/s/7ARBkhLl4mYunVg @DeepDiver1975 @ahmedammar @PVince81 @guruz @pmaier1 @ckamm |
|
These errors show something not quite right with your setup: “unable to parse zsync file” ... Those should really never happen. |
|
Whatever is the setup, it should work.. windows, linux, mac. I should be able to build a client from given branches and run flawlessly. Maybe the problem is server on mac. Might check it later. But other errors are not about unable to parse zsync file.. |
|
How will you explain, that master branch of client passes the 3 integrations tests, but delta-sync branch fails? I doubt it is my setup in that case (and both are agains server with zsync support) |
|
Would have been great if you ran these integration tests months ago when my pull request was made, no idea how stuff has diverged since ... All I’m saying is those errors at the validity of the zsync file itself is far from normal. Knowing your setup would help track down and actually fix the problem. |
|
What setup details do you have in mind?
|
|
That setup is similar to mine, did you run the unit tests? Where can I find the test script? |
|
seems @orzeech had exactly the same problem |
|
https://github.com/owncloud/smashbox/blob/master/lib/test_deltamove.py Make sure that in etc/smashbox.conf you have similar configuration. Additionaly please mind that for master branch, Lets move this conversation to the email for details - piotr@owncloud.com I really like this delta sync work, and I would love to merge the server side of it. Your code quality is very good (for enterprise we would need to optimize it, but for community use it is fine). If we pass these 3 integration test_nplusone, test_deltamove and test_basicSync I happily merge this PR. smashbox git:(delta_test) ✗ cat etc/smashbox.conf |
|
➜ client-build-mac make test 91% tests passed, 2 tests failed out of 23 |
|
I cannot reproduce "unable to parse zsync file”. But I do see 400 replies to MOVE requests. On first look it seems like the MOVE is triggered too early. This was testing a 20 000 000 bytes file being increased in size by 1 000 000 bytes: Also the chunk sizing is odd. From the logs it looks like this is what happens meaning that chunks overlap: And the server seems to expect 2048925 bytes, so 1024*1024 + 1 000 000; but that plus 19922944 doesn't add up to the expected 21 000 000. I can look at this more on Monday. |
|
This one looks interesting also |
|
I expect to have the upload issues fixed with the owncloud/client#6382 PR - that would also cover the assert failure you mentioned. |
|
@DeepDiver1975 @PVince81 @pmaier1 I did another round of tests - it passes test_deltamove.py in smashbox, and this is enough for me to confirm that delta works correctly and as expected. Things still not working as of owncloud/client#6382 :
Please bring this baby in 👍 |
|
This feature will be in the upcoming 2.6 alpha release. |
Description
This commit adds the required server-side support for delta-sync.
The basic approach is to store zsync metadata files in a folder called
files_zsync/which mirrors the structure and layout of thefiles/folder but appends.zsyncto the metadata files. These files can be requested by the client via a new routedav/files/$user/$path?zsync. They can also be deleted using the same route. This is implemented using a newServerPlugincalledZsyncPlugin.Filesystem hooks are used to mirror any
move/copy/deleteoperation on the base file onto the metadata file.The upload path is implemented by modifying the
ChunkingPlugin, the chunk files are now assumed to be named as the offsets into the original file. Special handling is done when a chunk named.zsyncis found, including copying the contents to thefiles_zsync/folder. This is to ensure that both the metadata and the actual file are put in place atomically, as part of the finalMOVErequest. The implemenation adds a new classAssemblyStreamZsyncwhich extendsAssemblyStreamwith additional support to fill in the data between chunk offsets from abackingFile.A new
zsynccapability is added to the files app, which can be checked by the client to know if delta-sync is supported or not.Related Issue
#16162.
How Has This Been Tested?
Mostly tested by using the owncloud command line and macOS client applications. With some randomly generated files using
ddand various modifications to those files:Types of changes
Checklist: