Skip to content
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

Implement parallel uploads to a single backend #3684

Merged
merged 23 commits into from Mar 19, 2019

Conversation

Projects
None yet
5 participants
@seantempleton
Copy link
Contributor

seantempleton commented Mar 6, 2019

The BackendUploader was heavily refactored to support concurrent uploads to a single backend. The functionality of encrypting, hashing, and creating index volumes were moved to the DataBlockProcessor and SpillCollectorProcess to allow the upload threads to continuously upload data rather than having to stop and encrypt data for example.

  • The IBackend and IStreamingBackend interfaces were updated to change the Put() function to return a Task. All backend implementations were updated to support the new signature.
  • Added a new option, asynchronous-concurrent-upload-limit to control how many uploads are allowed to happen concurrently. Defaults to 4.
  • Added a new class to track the progress of all concurrent uploads and report it to the BackendProgressUpdater as well as throttle the uploads if necessary.

Resolves #2026

seantempleton added some commits Feb 10, 2019

Move upload related code from BackendHandler to BackendUploader
As the BackendUploader is in charge of uploads the code has been moved
there to centralize the responsibility and prepare for parallel uploads.
Move encrypting, hashing, and creating index volumes out of BackendUp…
…loader

Encrypting, hashing, and creating index volumes are now done
in the DataBlockProcessor and the SpillCollectorProcess. This allows
uploads to always be transferring data and not have to stop to create
a file, encrypt data, etc.
Run each upload on a separate thread
Added a small Worker class, private to the BackendUploader, to track the
upload tasks and the backend for each task.
Update AlternativeFtp Backend for async Put
Removed System.Net.FtpClient and added the FluentFTP Nuget package which
is the new name of System.Net.FtpClient.
Add asynchronous-concurrent-upload-limit option
This new option limits the number of concurrent uploads allowed when
performing asynchronous uploads. The default value is 4.
Dynamically throttle and track the progress of concurrent uploads
Changed AddBackendEvent() in ResultClasses to allow the option to not update
the backend progress. This allows the  BackendUploader to log the act of starting
an upload without changing the backend progress. The backend progress is now
handled by a new class, FileProgressThrottler, which will determine the transfer
rate of all uploads currently in progress.

This class also handles throttling the upload streams. The algorithm will try
to decrease or increase the streams throttle rates as appropriate by changing
the value based on the over/under amount or based on a percentage to prevent
throttling one stream directly to zero.
@duplicatibot

This comment has been minimized.

Copy link

duplicatibot commented Mar 6, 2019

This pull request has been mentioned on Duplicati. There might be relevant details there:

https://forum.duplicati.com/t/backblaze-b2-slow-upload/4128/12

@warwickmm

This comment has been minimized.

Copy link
Contributor

warwickmm commented Mar 10, 2019

I finally made it through all the changes (it was easier looking at them one commit at a time) and it looks pretty reasonable to me. Nice work. Hopefully others will find time to look at this as well, since the number of changes isn't trivial.

It seems that users occasionally experience issues where (presumably after an interrupted or cancelled operation) their local database and remote files become out of sync. Were you able to perform any kind of testing to see how interruptions are handled and reported?

@verhoek

This comment has been minimized.

Copy link
Contributor

verhoek commented Mar 10, 2019

I finally made it through all the changes (it was easier looking at them one commit at a time) and it looks pretty reasonable to me. Nice work. Hopefully others will find time to look at this as well, since the number of changes isn't trivial.

I started looking into it today a bit, so far it looks OK but as you said a lot of changes + I'm unfamiliar with this part of the code and well, it isn't easy for me to dissect the way it's written (the old code). I do like that some parts got moved to places where they apparently make more sense.

@seantempleton

This comment has been minimized.

Copy link
Contributor Author

seantempleton commented Mar 13, 2019

How much weight is placed on Codacy? Is it required that every issue raised is fixed? For instance it says there are several member variables that are not used, but they definitely are. And it wants BackendUploader to implement IDisposable because of the CancellationTokenSource. Reading about that the recommendation is you really only need to dispose it if you are using a linked CancellationTokenSource.

@warwickmm

This comment has been minimized.

Copy link
Contributor

warwickmm commented Mar 14, 2019

I'd take the Codacy issues as just suggestions. There are times where they are clearly false flags, and some things are very subjective. Sometimes, a simple refactoring raises existing issues that I would rather not touch in a particular pull request. However, there are times where the issues raised are quite meaningful, so I would take a look at those. Hopefully reviewers are looking at the Codacy results as well, and will point out the ones that they think are important.

@seantempleton seantempleton force-pushed the seantempleton:parallelupload branch from d283db9 to c1371b0 Mar 14, 2019

seantempleton added some commits Mar 4, 2019

Properly catch exceptions from an upload and cancel all other uploads
If a backend throws an exception while uploading, cancel other uploads and
throw an exception from the BackendUploader. Exceptions from cancelled
uploads are caught and not thrown so as to not spam the user with useless
information.

seantempleton added some commits Mar 8, 2019

Fix code review comments and other minor changes
- Rename indexVolume to indexVolumeWriter in DataBlockProcessor to reduce
confusion.

- When flushing the remaining uploads, check the tasks for exceptions in
completion order rather than waiting for them all at once. If there is an
exception all other uploads are cancelled anyway.

- Remove setting the operation progress as Backup_WaitForUpload in the
BackendUploader since the BackupHandler sets it.

- Use an exception filter in BackendUploader.

- Use Options property MaxUploadPrSecond instead of getting the raw value.
Check upload throttle speed every 2 seconds
The upload throttle speed was being checked on average once every
5 milliseconds.

Changed a few variable names.

@seantempleton seantempleton force-pushed the seantempleton:parallelupload branch from c1371b0 to 19c1c90 Mar 14, 2019

@kenkendk

This comment has been minimized.

Copy link
Member

kenkendk commented Mar 17, 2019

Wow, large refactoring and update. You even fixed some of the backends and made them really async instead of just returning Task.FromResult(true). I do not fully understand the updated upload process, but I do not see any reason it should not work.

For Codacy, I think @warwickmm has the right attitude. It suggests improvements, but they do not always make things better, just stricter.

The Put method should be called PutAsync to follow the .Net conventions ? Are there reasons not to use the Async suffix here? I do not see a need for a non-async Put, as the caller can just use PutAsync(...).Wait(); to synchronize immediately.

Other than taht, I am good with merging this PR.

For future PRs we could consider:

  • Should we change IBackend to use async for all methods, like GetAsync, ListAsync etc?
  • Should we remove the IStreamingBackend and just have a SupportsStreaming property ?
  • The Utility.AsyncHttpRequest should be removed. It was added to support proper timeout and cancellation to the HTTP requests, but since we have async methods, this is not needed anymore.
@seantempleton

This comment has been minimized.

Copy link
Contributor Author

seantempleton commented Mar 17, 2019

I do not fully understand the updated upload process, but I do not see any reason it should not work.

If you have any specific questions I'd be happy to answer them. This is a bit simplified but the BackendUploader is mostly the same functionality wise but instead of running each upload through the RunOnMain function, it starts a task. Then some code was shuffled in from BackendHandler and other code shuffled out to the DataBlockProcessor and SpillCollectorProcess.

The Put method should be called PutAsync to follow the .Net conventions ? Are there reasons not to use the Async suffix here? I do not see a need for a non-async Put, as the caller can just use PutAsync(...).Wait(); to synchronize immediately.

I agree, I will change it to PutAsync.

I like those future changes to consider. I was wondering if I could swap out using async/await where AsyncHttpRequest was used, but decided I was changing enough things already.

Thanks for the comments kenkendk!

@seantempleton seantempleton force-pushed the seantempleton:parallelupload branch from d63de47 to b02b19e Mar 17, 2019

@warwickmm

This comment has been minimized.

Copy link
Contributor

warwickmm commented Mar 18, 2019

I ran my tests again on a variety of backends and most of them (Amazon Cloud Drive, B2, Box, Dropbox, Google Cloud Storage, Google Drive, Jottacloud, Local, Microsoft One Drive, OpenDrive WebDAV, SFTP) ran without issues.

However, my tests fail with the Mega backend. The initial backup returns an exit code of 1 instead of 0. When running the initial backup from the UI, it appears that it ran fine when in fact only a dlist file appears in the backend folder (perhaps we're running into issue #3673 here?). The logs show that an "Object reference not set to an instance of an object" error was encountered at some point. A subsequent backup run yields the "Found 1 remote files that are not recorded in local storage, please run repair" error.

These tests pass when using the most recent master (38dfdff), so I'm thinking that perhaps there's a small issue introduced in this branch.

Fix MegaBackend null reference exception
null was being passed to UploadAsync instead of an instance of an IProgress class.
MegaApiClient does not check if IProgress is null before using it.
@seantempleton

This comment has been minimized.

Copy link
Contributor Author

seantempleton commented Mar 18, 2019

I have no way of testing it but the changes for the Mega backend were trivial and I think I tracked the issue down. I was passing null to their upload function instead of an instance of their IProgress class.

@warwickmm

This comment has been minimized.

Copy link
Contributor

warwickmm commented Mar 18, 2019

My test suite is now passing. 😄

@kenkendk kenkendk merged commit 57b2818 into duplicati:master Mar 19, 2019

2 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.