-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Implementation of delta-sync support on server-side. #29404
Conversation
db9ebc6
to
f304357
Compare
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks really good. Extra ❤️ for all the unit tests. Nice!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this controller used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got that - but who is calling this? desktop client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay - can we move this to dav? We shall only use dav as api between clients and server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
|
da3a039
to
4701349
Compare
Util::connectHook('OC_Filesystem', | ||
'write', | ||
$this, | ||
'deleteZsyncMetadata'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.zsync
to 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 newServerPlugin
calledZsyncPlugin
.Filesystem hooks are used to mirror any
move/copy/delete
operation 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.zsync
is 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 finalMOVE
request. The implemenation adds a new classAssemblyStreamZsync
which extendsAssemblyStream
with additional support to fill in the data between chunk offsets from abackingFile
.A new
zsync
capability 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
dd
and various modifications to those files:Types of changes
Checklist: