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

ahmedammar · 2017-10-27T23:20:09Z

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.

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. 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, 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 of the zsync metadata file is done in a seperate thread since this is a cpu intensive task, ensuring main thread is not blocked.

Added configuration and command-line options to gui and cmd applications. Delta-sync is disabled by default, this can be changed once more testing is done.

CLAassistant · 2017-10-27T23:20:15Z

All committers have signed the CLA.

ckamm

Review of first third.

ckamm · 2017-11-01T14:24:22Z

Minor: Place this next to the other bitfields to keep SyncFileItem size small.

Also please add documentation about what these fields mean. (we don't have docs everywhere we'd like, but make an attempt to good documentation on new code and changes)

ckamm · 2017-11-01T14:32:16Z

Add:

adjustTotalsForFile is called during the treewalk phase to collect the initial total size and file count. updateTotalsForFile is called at most once per item during propagation to adjust them when new information has become available.

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.

ckamm · 2017-11-01T14:33:32Z

If this member isn't necessary for other reasons it might be worthwhile to get rid of it and pass the size as an argument in the updateTotalsForFile signal/call. We have a lot of SyncFileItems alive at the same time and have run into memory issues before. You may even be able to drop _isZsync that way -- unless it's necessary in the follow up commit that I haven't read yet.

ckamm · 2017-11-01T14:39:03Z

Possible bug: The getter returns the min size in bytes, the setter takes the size in megabytes?

If the getter and setter use different units, I suggest changing that.

ckamm · 2017-11-01T14:40:29Z

Important: Is compiling the zsync code into the owncloud client compatible with their license and our license?

Yes, already checked by @hodyroff. owncloud/core#16162 (comment)

Also for the code you copied (vs linked) like read_stream_write_blocksums @hodyroff ?

Even if this is fine license wise, it is bad to copy in the dependency. Maybe it makes sense to have it bundled for Win and Mac (it could also simply be built there as separate file), but Linuxes ship this library. It is bad to duplicate the code, we discussed this before ;-) .

What is the plan? Are you planning to bundle it in the final source ball? I think it would be better to not do that.

Are you talking about zlib specifically? It's actually not even really used, I can strip out all the support from zsync, I only did that to help me compile initially, and reduce any major changes to zsync sources.

Done, stripped zlib from zsync using preprocessor flag.

Not zlib specifically, also zsync

I really don't understand the issue, the source isn't bundled, it's a git submodule. There are some needed changes to zsync for it to support what is needed so I don't see any other way around that at this point ...

ckamm · 2017-11-01T15:33:32Z

This check seems odd, since file isn't used below. You could check _previousSize > 0? If you do want to keep it, produce an error if (!file.open(...)) and de-indent the happy case.

This is just to check that we have a file to seed from, otherwise no point doing delta-sync. I removed the else and removed some duplicate code since it can fall-through. Hard to do that if I de-indent the happy case, let me know your thoughts now.

I didn't see the fallthrough, makes sense then.

You shouldn't have to go to the filesystem to check whether there's a base file since the previous phases have already discovered all of that - hence I suggested just checking _previousSize. But it's a minor thing.

checking _item->_previousSize > 0 instead now, fixed.

ckamm · 2017-11-01T15:37:00Z

Is this really a warning or something that will regularly come up? I.e. does the server provide a zsync control file for each file?

qCInfo is probably more appropriate, fixed.

ckamm · 2017-11-01T15:37:49Z

unnecessary check unless something weird is going on in GetFileZsyncJob

I think this was left-over from old debug. Removed.

ckamm · 2017-11-01T15:40:28Z

Unfortunate large chunk of duplicated code. Get rid of it if at all possible. What are the differences between GetFinished and ZsyncFinished -- can we arrange such that there aren't any?

Unfortunate indeed, slotGetZsyncFinished is gone now.

ckamm · 2017-11-01T15:45:27Z

What? How can that be right? A zsync-downloaded can still create a local conflict - why would the method of data transfer make the conflict go away?

Not sure why I needed that now, I vaguely remember ._conflict files being created and this being the reason but I don't actually seem to need it now. Removed.

ogoffart

I apologize if i say stuff that were already said, for i just read the diff and not the previous comment that were made.

Very good work, thank you.

Is there any way that prevent the meta-data and the data to be out of sync because of a race condition:
What if the file on the server changes right after the metadata was downloaded? We need to make sure that the etag of the metadata matches that of the the file. Are you doing that?

Also, is there no cases where we should simply fallback to the normal download or upload if we detect that the file has changed too much?

ogoffart · 2017-11-15T13:12:26Z

I also think it is better not to have the setting in the UI.
It should simply be enabled by default with a sensible size.
We can keep a hidden setting in the config file.

ogoffart · 2017-11-15T13:13:39Z

Why do you need this?
I was under the impression that we already compile with large file sypport.

ogoffart · 2017-11-15T13:17:47Z

Why are you using a fork of zsync?
Can your change be upstreamed in the official repo?
Linux distributions will want to use their package version of zsync.

The repo owner of zsync is not that responsive, and some changes are very specific to the needs. For example, changes needed when uploading chunks, that's completely unsupported by zsync.

So that means that owncloud has to maintain this fork :-/
And also that nobody is really reviewing these changes.

Not much I can do about that.

ogoffart · 2017-11-15T13:22:06Z

The best would be nice if zsync itself was shipping a cmake module. Otherwise we could still have a cmake module in our repo so this does not have to be done like that (and could potentially be easily disabled)
Would just be better. Does not need to be done in the first release.

ogoffart · 2017-11-15T13:25:57Z

Any performance impact on the server?
(We had problem previously that some property were slow to check on the server and adding them here would make the discovery much slower)

It's just a check on if a file exists, which should be using the file cache, should be fast.

ogoffart · 2017-11-15T14:57:59Z

s/fname/_zsyncFilePath/

No need to converto to 8bit and back again.

ogoffart · 2017-11-15T14:58:54Z

wondering why you need to do that. Can you not use the file descriptor from QFile directly?

Duplicating the handle was because I read that closing a file handle for a QFile is a bug: https://stackoverflow.com/questions/9465727/convert-qfile-to-file/16876527#16876527

ogoffart · 2017-11-15T15:00:42Z

Use QByteArray here (and no constData())

ogoffart · 2017-11-15T15:01:13Z

ogoffart · 2017-11-15T15:03:30Z

why not? if the server says it has zsync enabled why should it not work?

Because external drives are out of oC control, if they change then the metadata will be completely out-of-sync and corruption will ensue.

Then oC should not set that zsync is enabled for these.

Hmm, it's not that simple, oC doesn't have that much control over the process, the client has more, oC will not put the metadata if the client sends it for a file stored externally, but by that time the client has already done the cpu-intensive tasks of generating the metadata file and sent it. The client saves a lot by not doing that in the first place ...

ahmedammar · 2017-11-15T17:44:13Z

What if the file on the server changes right after the metadata was downloaded? We need to make sure that the etag of the metadata matches that of the the file. Are you doing that?

No idea how to do that to be honest, currently I rely on the checksum failing to catch this edge case.

Also, is there no cases where we should simply fallback to the normal download or upload if we detect that the file has changed too much?

This should be easy to do, but then the question is what is the threshold? >75%? >50% ?

ogoffart · 2017-11-16T13:03:52Z

Could be put in a unique_ptr

ogoffart · 2017-11-16T13:23:07Z

👎 This makes the test void.
The goal was to test that all the chunk files are there and they are named properly.

See the QCOMPARE(sourceFolder->children.count(), count); // There should not be holes or extra files later, how can that fail with the current code?

So i'd just change the 8 with a 16.

Yeah, that was on my TODO, to fix properly.

8 to 16 will not fix this problem, the problem is that count is no longer a valid approach because chunkId is not a simple auto incrementing number, it's the offset into the file now ...

I'll need to implement a better way to check that all the chunkId's to chunkSize match the range 0 to item->_size, with no holes. Similar in idea to what is done by PropagateUploadFileNG::updateRanges.

ogoffart · 2017-11-16T13:29:25Z

What if the file on the server changes right after the metadata was downloaded? We need to make sure that the etag of the metadata matches that of the the file. Are you doing that?

No idea how to do that to be honest, currently I rely on the checksum failing to catch this edge case.

Put the etag of the file in a header or somewhere in the metadata.
Then if the GET don't have the same etag, it means the file is no longer matching and you can retry.
And the PUT/MOVE will fail bacause of the If-Match

ogoffart

Another missing thing is the tests :-)

(I wish there was a way to make the zsync and upload code less mixed.
The code of the propagateuploadng for example becomes quite complicated.)

ogoffart · 2017-11-17T09:25:05Z

What errors do you get ? This should work.

ogoffart · 2017-11-17T09:26:34Z

use the inline initialization:

size_t _blocksize = 1048576; off_t _len = 0;

So you don't need to repeat that in the constructor.

ogoffart · 2017-11-17T09:30:18Z

Resume the transfer if the previous transfer aborted in the middle of the file (connection was cut)

What does the temporary file contains in this case?
In the normal download case, the temporary file is left on the disk and re-used for the next sync so we resume from that.

ogoffart · 2017-11-17T09:32:19Z

Ok good, that's small enough.

ogoffart · 2017-11-17T09:38:02Z

Do you have similar check for the delta sync?

This test exist because sometimes, the GET might end without error, but only contains a turncated file. (this happens sometimes when there is a gateway and the http chunk encoding is used)

Maybe if there is checksum check and that the file is correct, or the zsync algo detect corrupted patch files?

The final checksum check would definitely catch this.

ogoffart · 2017-11-17T09:39:18Z

Again, this might break resuming.

Again, same answer as before, if we error'd when parsing zsync then this file was simply created and contains 0 bytes.

ogoffart · 2017-11-17T09:42:23Z

QByteArray is default constructed. and NULL is specific to C.

Please actually initialize everything inline.
The old code did not do that because we only dropped compiler that did not support C++11 very recently. But new code should use the C++11 way.

ogoffart · 2017-11-17T09:44:08Z

Should you not check the DB entry for resuming, first?
Maybe then the metadata or some additional state need to be kept.

The ranges needed can't be known until the metadata is downloaded and seeded, the DB is checked after to see what's still needed/missing.

ogoffart · 2017-11-17T09:57:25Z

I'd rather you don't change the meaning of currentChunk for the normal (non zsync) upload.
This is going to cause problems, for example while resuming. (The logic in PropagateUploadFileNG::slotPropfindFinished will break)

ogoffart

please also run git-clang-format on the change

ogoffart · 2017-11-20T14:19:37Z

There is actually no reason to change that. The chunk is only used in the database for the old chunking algo. So please leave it as an int here.

ogoffart · 2017-11-20T14:21:00Z

Well, the problem is that this general settings page is already way too big, so adding more field in this tab is not a good idea at all.

ahmedammar · 2017-11-20T20:53:52Z

@ogoffart

I've made some more changes as you requested and ran git clang-format on the commit.

Put the etag of the file in a header or somewhere in the metadata.
Then if the GET don't have the same etag, it means the file is no longer matching and you can retry.

I will add this and the DB flag for metadata. I am about to travel to start my new job soon, I doubt I will be able to add any more work to this after Wednesday.

ahmedammar · 2017-11-22T09:45:33Z

When working on adding tests on the client side, I seem to have some bugs when tiny (1byte) modifications are made to the files. So I will need to come back to this when I have more time, the bugs must be somewhere in zsync code itself.

That and adding the original file etag when downloading are the main two blockers (functionally) right now.

ogoffart · 2017-11-23T13:44:35Z

( nitpick: i think you can do contents += contentCharp )

ogoffart · 2017-11-23T13:46:09Z

(nitpick, there is an overload of write that takes a char* i thin, so you do not need to create a QByteArray for it)

ogoffart · 2017-11-23T13:47:08Z

but this does not modify any bytes...

ahmedammar · 2017-11-23T14:18:12Z

OK I'm done with the automated testing. I really need to stop working on this now. I'll be back in a few weeks. Reachable via email/github.

ahmedammar · 2017-12-17T17:50:22Z

@ckamm @ogoffart @guruz @dragotin Can I know where this PR stands now? I had to rebase since some things changed upstream and merge was no longer possible. I'd really like to close this.

ahmedammar · 2017-12-17T18:34:59Z

@ogoffart please review last [fixup] commit, adding support for Etag checks during download.

ahmedammar · 2018-01-02T21:39:19Z

Bump.

ahmedammar · 2018-01-11T03:59:32Z

Bump.

ogoffart · 2018-01-04T11:43:20Z

@@ -498,7 +498,13 @@ bool SyncJournalDb::checkConnect()
        // local files and a remote discovery will fix them.
        // See #5190 #5242.
        if (major == 2 && minor < 3) {


This if is no longer required. (covered by the latter)

ogoffart

For some reason github can't show this pull request anymore. I always get the unicorn.
And i can't seem to add more comments.

ckamm · 2018-01-11T08:24:26Z

I'm having similar problems. Maybe open a new PR with the current state? But there were a lot of remaining open comments: If you can still load the page occasionally, can you archive it and upload it somewhere? I've not had the page load successfully for a good while.

…

On 11.01.2018 09:21, Olivier Goffart wrote: ***@***.**** commented on this pull request. For some reason github can't show this pull request anymore. I always get the unicorn. And i can't seem to add more comments. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6131 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAy3GpcY2DqR5_BGuJplAASOzp2RJffdks5tJcSBgaJpZM4QJuIM>.

ahmedammar · 2018-01-11T11:58:56Z

It does load, just takes a while due to all the comments, so multiple refresh of the page is needed. will create a new PR ...

…

On Jan 11, 2018, at 3:24 AM, ckamm ***@***.***> wrote: I'm having similar problems. Maybe open a new PR with the current state? But there were a lot of remaining open comments: If you can still load the page occasionally, can you archive it and upload it somewhere? I've not had the page load successfully for a good while. On 11.01.2018 09:21, Olivier Goffart wrote: > ***@***.**** commented on this pull request. > > For some reason github can't show this pull request anymore. I always get the unicorn. > And i can't seem to add more comments. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub <#6131 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAy3GpcY2DqR5_BGuJplAASOzp2RJffdks5tJcSBgaJpZM4QJuIM>. > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

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 seperate thread since this is a cpu intensive task, ensuring main thread is not blocked. This commit closes owncloud#179.

ahmedammar · 2018-01-11T12:33:22Z

Can only seem to load it on my mobile, not my laptop, closing this now ...

ahmedammar · 2018-01-11T12:39:50Z

See #6297.

guruz · 2019-03-20T11:01:04Z

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 9 times, most recently from 218ec16 to 9a30f6e Compare October 28, 2017 19:24

ahmedammar mentioned this pull request Oct 28, 2017

Allow remote-delta on sync files owncloud/core#16162

Closed

ahmedammar force-pushed the master branch 10 times, most recently from 7fd7b14 to a201a45 Compare October 29, 2017 14:37

ckamm self-requested a review October 30, 2017 09:54

ahmedammar force-pushed the master branch 6 times, most recently from aa21f0d to 0ed21aa Compare October 31, 2017 22:29

ckamm suggested changes Nov 1, 2017

View reviewed changes

ahmedammar force-pushed the master branch from 0ed21aa to 8b53484 Compare November 2, 2017 10:06

ahmedammar force-pushed the master branch from a4e417d to 74a1117 Compare November 15, 2017 14:21

ogoffart reviewed Nov 15, 2017

View reviewed changes

ahmedammar force-pushed the master branch from 014ce5f to 0719056 Compare November 15, 2017 18:45

PVince81 mentioned this pull request Nov 16, 2017

Sync only the file change, not entire file [$1,755] #179

Closed

ogoffart reviewed Nov 16, 2017

View reviewed changes

ogoffart reviewed Nov 17, 2017

View reviewed changes

ogoffart reviewed Nov 20, 2017

View reviewed changes

ahmedammar force-pushed the master branch from f6785d1 to b3670cb Compare November 20, 2017 20:27

ogoffart reviewed Nov 23, 2017

View reviewed changes

ahmedammar mentioned this pull request Nov 28, 2017

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

Open

ahmedammar added 2 commits December 17, 2017 12:18

Add Capabilities::zsyncSupportedVersion function.

6cc975f

Add support to update ProgressInfo item size.

babfdc7

ogoffart reviewed Jan 11, 2018

View reviewed changes

ahmedammar mentioned this pull request Jan 11, 2018

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

Closed

Uh oh!

Conversation

ahmedammar commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Oct 27, 2017 • 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!

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!

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 Nov 10, 2017 • 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!

ahmedammar Nov 2, 2017 • 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!

ahmedammar Nov 2, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

ahmedammar commented Oct 27, 2017 •

edited

Loading

CLAassistant commented Oct 27, 2017 •

edited

Loading

ahmedammar Nov 10, 2017 •

edited

Loading

ahmedammar Nov 2, 2017 •

edited

Loading

ahmedammar Nov 2, 2017 •

edited

Loading