Delta-sync upload fixes #6382
Merged
Delta-sync upload fixes #6382
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
On non block-aligned appends to local files overlapping upload ranges would be computed.
9 tasks
|
TestChunkingNG::testResume is failling but i don't know if it is a regression. Otherwise, just merge it. |
|
@ogoffart It's a regression I'll have to investigate. |
|
are you sure it is not related with assert fix? |
_rangesToUpload and _sent were adjusted when the PUT job started instead of at the end. Since the condition for whether to attempt the final MOVE was _rangesToUpload.isEmpty() this lead to the MOVE being done too early in some cases.
If existing chunks on the server needed to be deleted, the whole job could get stuck if the zsync metadata upload was running when the last delete job finished. The new code checks whether all delete jobs have finished instead of just checking for running jobs.
If the last upload finished before the zsync metadata upload was done we'd run into an assert. This fixes the assert and moves it to a better place.
No matter whether it's aligned to zsync blocksize or not.
ckamm
force-pushed
the
delta-upload-fixes
branch
from
91b72b5
to
90c2328
Compare
|
Fixed. The issue was that I adjusted |
|
I still have problems with delta sync when I manualy change the file - no idea what is wrong.
I upload a file: Then I modify one character inside the file and this happens: After the highlighned file I get error, and delete of zsync starts, and in next run the file in normaly uploaded. |
|
Tried This is also the last thing before long hang: On the server I get |
|
Tried I also confirmed, that delta worked correctly - and did delta
|
|
Additionaly, please note that if I move the file - all zsync metadata gets removed (?) and GET will fetch whole file. Server side errors for test with moving file: |
|
@mrow4a On #6382 (comment) - I've tried your specific test case but can't reproduce the "Unable to parse zsync file." error. Can you try again and check stderr?
|
|
Are you sure you run cmd with --deltasync flag? |
|
For the basic test yes (so we do indeed need more information about that parse error), but not for the smashbox test. With the flag I can reproduce errors on |
|
That took some digging! Here's why zsync gets stuck with the particular test data:
We either need smaller block sizes or to adjust the rolling checksum. Just increasing its width might already suffice. @ogoffart I think that fix is independent of this PR. |
|
👍 |
|
A simpler restatement of the problem: For files where every byte has value I can actually reproduce this particular problem with rsync! Note that 131072 is 2^17 and the largest block size that rsync allows. Probably for good reason? But since it's larger than 2^16 it already breaks with this particular test data. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I still need to add a bunch of unit tests but manual upload tests now pass much more reliably for me.
@mrow4a Possibly rerun your tests with this branch to verify.
@ahmedammar