Implementation of delta-sync support on client-side. by ahmedammar · Pull Request #6297 · owncloud/client

ahmedammar · 2018-01-11T12:37:32Z

This commit adds client-side support for delta-sync, this adds a new
3rdparty submodule gh:ahmedammar/zsync. This zsync tree is a modified
version of upstream, adding some needed support for the upload path and
other requirements.

If the server does not announce the required zsync capability then a
full upload/download is fallen back to. Delta synchronization can be
enabled/disabled using command line, config, or gui options.

On both upload and download paths, a check is made for the existance of
a zsync metadata file on the server for a given path. This is provided
by a dav property called zsync, found during discovery phase. If it
doesn't exist the code reverts back to a complete upload or download,
i.e. previous implementations. In the case of upload, a new zsync
metadata file will be uploaded as part of the chunked upload and future
synchronizations will be delta-sync capable.

Chunked uploads no longer use sequential file names for each chunk id,
instead, they are named as the byte offset into the remote file, this is
a minimally intrusive modification to allow fo delta-sync and legacy
code paths to run seamlessly. A new http header OC-Total-File-Length is
sent, which informs the server of the final expected size of the file
not just the total transmitted bytes as reported by OC-Total-Length.

The seeding and generation of the zsync metadata file is done in a
separate thread since this is a cpu intensive task, ensuring main thread
is not blocked.

This commit closes #179.

ahmedammar · 2018-01-11T12:43:13Z

webarchive of #6131 (since it unicorns a lot).

https://www.dropbox.com/s/5i0tdccn3dzbjlh/Implementation%20of%20delta-sync%20support%20on%20client-side.%20by%20ahmedammar%20%C2%B7%20Pull%20Request%20%236131%20%C2%B7%20owncloud%3Ac.webarchive?dl=1

ogoffart

I still haven't reviewed all of it.

There is quite a lot of jenkins faillure.

ogoffart · 2018-01-12T09:28:58Z

+        ws2_32
+    )
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_FILE_OFFSET_BITS=64")
+    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -D_FILE_OFFSET_BITS=64")


Why is this required?

It would be nice also if everything that is related to zsync lib was together so we could later split it in its own file.

Because of size_t usage and assumptions by zsync. See webarchive comments.

Regarding putting all the changes together in one place, I can do that, no problem.

ogoffart · 2018-01-12T09:33:21Z

+    int metaHandle = zsyncmeta.handle();
+    int tfHandle = zsynctf.handle();
+
+    zsync_unique_ptr<FILE> meta(fdopen(dup(metaHandle), "w"), [](FILE *f) {


Why are you not using QFile here, and everywhere you use raw file operation?
QFile has a constructor that opens a file descriptor as well.
I'd think this would lead to more readable code.

These functions were copied from zsync source as-is since they are not part of the exported api. Needed when generating a metadata file (part of their make cli tool). My main task was to integrate zsync, not re-write it.

Also, please see the archive of all the comments I linked, none of this is new.

ogoffart · 2018-01-12T09:38:24Z

+extern "C" {
+#include "librcksum/rcksum.h"
+#include "libzsync/zmap.h"
+#include "libzsync/sha1.h"


btw, Qt also has a way to compute the sha1.

Again, see previous comment.

ogoffart · 2018-01-12T09:39:48Z

    </widget>
   </item>
   <item row="2" column="0">
+    <widget class="QGroupBox" name="advancedGroupBox">


I still think there should be no UI addition.

Then you can feel free to remove it before the merge.

ogoffart · 2018-01-12T09:45:24Z


 namespace OCC {

+class GETJob : public AbstractNetworkJob


If you've got time, you could try tomake a separate commit that refactor GETJob and GETFileJob.
That'd make reviewer slightly easier.

My time is all up on this work now, it's taken way too long to get this far ...

ahmedammar · 2018-01-14T07:32:02Z

@guruz @ogoffart @dragotin @ckamm @DeepDiver1975 @PVince81
Based on the amount of time that has elapsed, and the amount of nit-picking happening, my guess is oC don't really want to merge this. That's fine, I can withdraw my PRs and just see if nextCloud want to take them as-is for free.

ahmedammar · 2018-01-14T21:07:34Z

Note: Jenkins failures were resolved after rebase on latest master ...

This commit adds client-side support for delta-sync, this adds a new 3rdparty submodule `gh:ahmedammar/zsync`. This zsync tree is a modified version of upstream, adding some needed support for the upload path and other requirements. If the server does not announce the required zsync capability then a full upload/download is fallen back to. Delta synchronization can be enabled/disabled using command line, config, or gui options. On both upload and download paths, a check is made for the existance of a zsync metadata file on the server for a given path. This is provided by a dav property called `zsync`, found during discovery phase. If it doesn't exist the code reverts back to a complete upload or download, i.e. previous implementations. In the case of upload, a new zsync metadata file will be uploaded as part of the chunked upload and future synchronizations will be delta-sync capable. Chunked uploads no longer use sequential file names for each chunk id, instead, they are named as the byte offset into the remote file, this is a minimally intrusive modification to allow fo delta-sync and legacy code paths to run seamlessly. A new http header OC-Total-File-Length is sent, which informs the server of the final expected size of the file not just the total transmitted bytes as reported by OC-Total-Length. The seeding and generation of the zsync metadata file is done in a separate thread since this is a cpu intensive task, ensuring main thread is not blocked. This commit closes owncloud#179.

mrow4a · 2018-01-16T10:44:59Z

Is this PR ready from the client side perspective? @guruz @ogoffart

ckamm · 2018-01-17T15:21:16Z

@mrow4a Not ready in my opinion. See below.

@ahmedammar I understand your frustration. I genuinely think you've done an amazing job responding and fixing things based on the hundreds of review comments so far. The old PR ended up being too much for github because you were extraordinarily willing to discuss, adjust and fix all of the issues and nitpicks that came up. 👍

Unfortunately this is a large change to a critical component of the sync client and thus everyone seems a bit extra picky and there are worries about reliability. In addition it seems that, with you busy and probably not interested in continuing with this patch after it lands, the existing maintainers will have to fix whatever issues pop up. That makes it necessary for at least two people to understand what's going on well enough to be able to work on it efficiently.

My main issues at this point are:

reliability on Windows

With this patch zsync will be responsible for a lot of the filesystem operations. One wouldn't expect it, but basic stuff for dealing with files can be surprisingly hard to get right (I'm thinking of mtime timezone issues, locked files, shared access permissions, etc). And I'm convinced we'll have several regressions in this area.

reliability generally

While there are now two basic tests (yay!), there will be ample of bugs left to find. Since this patch mostly deals with uploads and downloads these are more likely than usual to be corruption/data loss kind of bugs.

zsync integration generally

The current PR copies some of zsync's code into the client, other code we link against. In any case, the version we link against is a custom modified version of zsync. Essentially zsync becomes part of the client project, but resides in a different repository while being mutually dependent. If there's no strong reason to keep these separate I'd just fold zsync into the client repository to keep things easy.

review age

My review was months ago and there have been many changes since (which is great and has improved things!). But still, I'll need to go over things again to get an up-to-date impression of what's going on.

Path forward?

I'd suggest something like this:

Decide whether we're willing to make the investment of adopting this PR. It's pretty cool, but there are costs.
Review again with a focus on "does this addition break non-delta-sync codepaths?". Make any changes necessary to reduce risk of accidentally breaking default syncing.
Merge in a state where the feature is disabled by default.
Have 1-2 people clean up the remaining loose ends.

And then we need to stabilize. That means adding lots of tests, enabling it on dev machines, pinging interested users whether they're up for testing in low-risk scenarios. Eventually enable it by default.

ahmedammar · 2018-01-18T02:07:18Z

First, thanks for the detailed response and sharing your views so succinctly.

All the points made by @ckamm are valid but I just wanted to respond to a few things.

I want to reaffirm that I am willing to continue to maintain the code after the code lands, I might be busy with work but that doesn't mean I can't continue to maintain this in my spare time.
Regarding folding the zsync code in, I don't think that's a good idea since you'd lose the git commit history. As a compromise, if oC would fork my zsync tree, we could use that as the submodule reference instead.
Merging in a default disabled state is actually how it's implemented today and also the reason I made the GUI addition (to allow users to enable/test it).
I have no problem in helping clean-up any loose ends or adding more tests but I just want to see some positive movement from oC side before investing more time.

tflidd · 2018-01-23T15:02:47Z

From a user's perspective, this would be really a cool new feature. We've never got so close to this feature. There has been such a demand that there will be some people ready to test it. It will certainly require some development costs as well but what better feature to invest in for a sync solution?

SamuAlfageme · 2018-01-23T15:08:15Z

@tflidd it is indeed; we're thinking about generating some testpilotcloud installers as well as instructions for the server patch to get community to start trying this out as soon as possible to get feedback. We'll announce soon in https://central.owncloud.org.

PVince81 · 2018-01-23T15:09:43Z

I don't think a server patch will be needed. We'll merge the server PR and release it in the upcoming 10.1 pre-alpha/alpha so people can use that for testing

ogoffart · 2018-01-26T11:57:31Z

@ahmedammar Have you had any contact with the zsync maintainer? Why did you not create pull requests there?
(Sorry if you already said that earlier, but i don't have easy access to previous comment)

(In the case we cannot upstream the change, we will make a fork of zsync on github in the owncloud organisation)

Edit: I actually see that you made a pull request. ( cph6/zsync#2 ) But why is it closed?

ahmedammar · 2018-01-26T12:17:38Z

I reached out to him, and got no response.

ckamm · 2018-01-29T09:35:39Z

@ahmedammar There was a meeting where we talked about this PR. Upcoming steps are:

Final review round with an eye on reducing risks of breaking unrelated behavior when this feature is disabled. Me and @ogoffart will do that.
Merge into a branch where the three of us can tie up loose ends. We'll also build experimental binaries for people to play with.
Merge into master.

At the same time there'll be another attempt to reach out to the author of zsync. Depending on how this goes we might upstream patches or create a fork in the owncloud organization.

@SamuAlfageme @ogoffart @guruz

ckamm

I have gone through the diff again, looking primarily at the zsync-disabled branches.

There's one inline comment about progress updating where something seems off.

Besides that the only big change to existing paths is in the upload chunking. The chunk numbering the servers receive will be very different. I'll need to go through the updated logic again tomorrow.

ckamm · 2018-01-29T13:08:19Z

@@ -229,7 +245,7 @@ void ProgressInfo::setProgressComplete(const SyncFileItem &item)
    _currentItems.remove(item._file);


needs to move below the if or _totalSizeOfCompletedJobs won't update correctly. edit: done

ckamm · 2018-01-29T13:09:34Z

+     * Example: With delta-sync, the actual size of the download will only
+     * be known during propagation - this function adjusts the total size
+     * to account for it.
+     */


for later: This might need even more documentation: item._size must be the old size when this is called, different from newSize which isn't obvious to me. edit: done

ckamm

Looks good to me.

ckamm · 2018-01-30T09:47:49Z

+    bool _removeJobError = false; /// if not null, there was an error removing the job
+    bool _zsyncSupported = false; /// if zsync is supported this will be set to true
+    bool _isZsyncMetadataUploadRunning = false; // flag to ensure that zsync metadata upload is complete before job is
+    quint64 _bytesToUpload; // in case of zsync upload this will hold the actual bytes to upload, normal upload will be file size


later: I was wondering what would happen to _bytesToUpload for the case of resumes. It turns out it works fine since it's always used in the context of "total bytes that need to arrive on the server before we're done".

So if the file size is 10 and a previous job uploaded 6, this member would be 10 even though the actual number of bytes to upload for this job is just 4. Rename / documentation change probably.

edit: done

ckamm · 2018-01-30T09:48:07Z

    quint64 _sent = 0; /// amount of data (bytes) that was already sent
    uint _transferId = 0; /// transfer id (part of the url)
-    int _currentChunk = 0; /// Id of the next chunk that will be sent
+    int _currentChunk = 0; /// id of the next chunk that will be sent


later: _currentChunk -> _currentChunkStart ? edit: done

ckamm · 2018-01-30T10:04:47Z


-    if (_sent > _item->_size) {
+    if (!_rangesToUpload.isEmpty())
+        _currentChunk = _rangesToUpload.first().start;


later: Potentially redundant, _currentChunk is also set in startNextChunk() - only used in logging, it seems. edit: done

ckamm · 2018-01-30T10:06:56Z

    propagator()->_activeJobList.append(this);

+    _zsyncSupported = isZsyncPropagationEnabled(propagator(), _item);
+    if (_zsyncSupported && _item->_remotePerm.hasPermission(RemotePermissions::HasZSyncMetadata)) {


later: In the case where this is false, one can still have _zsyncSupported be true and use the corresponding code paths below. I haven't looked in detail whether that's correct.

ckamm · 2018-01-30T10:19:16Z

I've merged this PR manually into the delta-sync branch (there was a trivial conflict).

ahmedammar · 2018-02-01T06:18:45Z

@ckamm thanks for the review and pulling into a branch but how should I address the things mentioned above after you've already merged into a branch?

ckamm · 2018-02-01T09:32:04Z

@ahmedammar Let's continue working on it collaboratively in the branch by making small PRs against it. I've committed a couple of minor fixes directly but my larger changes will also go through pull requests for review.

orzeech · 2018-02-05T10:29:45Z

@ahmedammar @ckamm Hi, I've been testing this code a bit. I've set up two testing environments and I've found some bigger and smaller problems. From progress bar not being updated correctly to total crash of synchronization after synchronizing many times a group of files from a delta-sync enabled and regular client one after another. There was also a bug when delta-sync was not triggered when adding just the last byte to a file. How do I go on with reporting those? Otherwise it's bringing great speed improvements in some cases.

ahmedammar · 2018-02-05T12:21:04Z

@orzeech are you testing the delta-sync branch that @ckamm is now working in? Since some of these things should be fixed there. Also about reporting issues, reproducibility of the issue is quite important for me to duplicate the situation. Thanks for testing!

orzeech · 2018-02-05T13:11:50Z

I will check on this new branch and get back to you soon

guruz · 2019-03-20T11:00:54Z

This feature will be in the upcoming 2.6 alpha release.
Meanwhile you can try it inside the daily builds 2.6.x https://download.owncloud.com/desktop/daily/

ahmedammar force-pushed the master branch 2 times, most recently from 7f97a0e to d4011ab Compare January 11, 2018 12:39

ahmedammar mentioned this pull request Jan 11, 2018

Implementation of delta-sync support on client-side. #6131

Closed

ahmedammar added 2 commits January 11, 2018 07:44

Add Capabilities::zsyncSupportedVersion function.

e7ef50a

Add support to update ProgressInfo item size.

9586121

ogoffart reviewed Jan 12, 2018

View reviewed changes

ahmedammar force-pushed the master branch from d4011ab to 8efa931 Compare January 12, 2018 12:32

ahmedammar force-pushed the master branch from 8efa931 to 2829228 Compare January 14, 2018 21:06

ahmedammar force-pushed the master branch from 2829228 to 520db37 Compare January 15, 2018 03:00

ahmedammar force-pushed the master branch from 520db37 to 12aeb89 Compare January 15, 2018 15:35

ckamm reviewed Jan 29, 2018

View reviewed changes

ckamm approved these changes Jan 30, 2018

View reviewed changes

ckamm closed this Jan 30, 2018

SamuAlfageme mentioned this pull request Jun 18, 2018

Add "Settings > General: Experimental" section #6590

Closed

4 tasks

maverick74 mentioned this pull request Nov 15, 2018

Delta-sync, or sync only changed bytes in a file nextcloud/server#417

Open

		@@ -229,7 +245,7 @@ void ProgressInfo::setProgressComplete(const SyncFileItem &item)
		_currentItems.remove(item._file);

Uh oh!

Conversation

ahmedammar commented Jan 11, 2018

Uh oh!

ahmedammar commented Jan 11, 2018

Uh oh!

ogoffart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmedammar Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmedammar Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmedammar commented Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahmedammar commented Jan 14, 2018

Uh oh!

mrow4a commented Jan 16, 2018

Uh oh!

ckamm commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

reliability on Windows

reliability generally

zsync integration generally

review age

Path forward?

Uh oh!

ahmedammar commented Jan 18, 2018

Uh oh!

tflidd commented Jan 23, 2018

Uh oh!

SamuAlfageme commented Jan 23, 2018

Uh oh!

PVince81 commented Jan 23, 2018

Uh oh!

ogoffart commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahmedammar commented Jan 26, 2018

Uh oh!

ckamm commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ckamm left a comment

Choose a reason for hiding this comment

Uh oh!

ckamm Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckamm Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ahmedammar Jan 14, 2018 •

edited

Loading

ahmedammar Jan 14, 2018 •

edited

Loading

ahmedammar commented Jan 14, 2018 •

edited

Loading

ckamm commented Jan 17, 2018 •

edited

Loading

ogoffart commented Jan 26, 2018 •

edited

Loading

ckamm commented Jan 29, 2018 •

edited

Loading

ckamm Jan 29, 2018 •

edited

Loading

ckamm Jan 29, 2018 •

edited

Loading

ckamm Jan 30, 2018 •

edited

Loading

ckamm Jan 30, 2018 •

edited

Loading

ckamm Jan 30, 2018 •

edited

Loading