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

Parallel download #7429

Merged
merged 1 commit into from
Aug 30, 2019
Merged

Parallel download #7429

merged 1 commit into from
Aug 30, 2019

Conversation

kfarmer-msft
Copy link
Contributor

Adds parallel download support from blobs given either destination Stream or FileInfo. Some minor renames of parallel transfer options to reflect a more generic view of things.

).ContinueWith(
t =>
{
stream.Flush();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this switch between Flush/FlushAsync depending on the async mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to recall, but if this is the case I recall, it really started to do funky things with the return type -- multiple-nested Task<>, and so forth.

Easy enough to experiment with later.

@@ -114,8 +119,9 @@ internal static class Append

internal static class Block
{
public const int DefaultParallelUploadCount = 4; // TODO What should the value really be? Can we get rid of it with a different dispatch algorithm? (probably yes)
public const int DefaultConcurrentTransfersCount = 4; // TODO What should the value really be? Can we get rid of it with a different dispatch algorithm? (probably yes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked about this and was told the magic number is 5... for no better reason than everybody else uses the magic number 5. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnRusk .. this is something you'll want to be looking at.

I chose 4 because once upon a time that was the limit for IE without hacking the registry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was talking to John about this and he said that 4 is actually pretty low. Maybe we should up it to 8. This is to increase the amount of time it takes for uploading and at the same time the number is still low enough to be fair to other connections as well. It doesn't have to be in this PR, we can always make another PR to increase this number.

@@ -668,6 +669,573 @@ protected virtual BlobBaseClient WithSnapshotImpl(string snapshot)
}
#endregion Download

#region Parallel Download
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need both Download and Parallel Download? Wouldn't Parallel Download suffice for everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My view was that parallel depended on sequential. There's certainly a limited below which sequential is the way to go. Parallel is primarily a large-file thing, both in terms of how it handles memory and in having an extra call to detect the size and determine whether to use the non-parallel approach.

Another point is that parallel download depends on being handed the destination, rather than being returned as part of BlobDownloadInfo (hence why I'm only returning BlobProperties). They have different signatures because of that.

@amnguye amnguye merged commit bf777bb into Azure:master Aug 30, 2019
@kfarmer-msft kfarmer-msft deleted the parallel-download branch August 30, 2019 23:24
JoshLove-msft pushed a commit to JoshLove-msft/azure-sdk-for-net that referenced this pull request Sep 10, 2019
openapi-bot-sh-dev bot pushed a commit to test-repo-tih/azure-sdk-for-net that referenced this pull request Oct 8, 2019
Update examples (Azure#7429)

* Copy compute.json and runCommands.json from 2019-03-01 to 2019-07-01

* changes to add publicIpAddressVersion field (Azure#7173)

* Add diskEncryptionSet in swagger compute-2019-07

* resolve semantic conflicts

* Fix model conflicts

* Resolve readme

* Resolve readme

* Resolve description conflicts

* Improve description

* Fix spell error

* Add some examples.

* fix model error

* Update examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants