Updated Default Concurrency count #54428
Conversation
nickliu-msft
requested review from
amnguye,
jalauzon-msft,
jaschrep-msft and
seanmcc-msft
as code owners
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
1 similar comment
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
nickliu-msft
changed the title
|
|
||
| private readonly ArrayPool<byte> _arrayPool; | ||
|
|
||
| private readonly int DefaultConcurrentTransfersCount = Math.Min(Math.Max(Environment.ProcessorCount * 2, 8), 32); |
There was a problem hiding this comment.
Intentionally had to do this since Math.Clamp() is not supported for netstandard2.0 and earlier
| { | ||
| public const int DefaultConcurrentTransfersCount = 5; | ||
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| public const int DefaultConcurrentTransfersCount = LegacyDefaultConcurrentTransfersCount; |
There was a problem hiding this comment.
I get that this is to avoid altering a ton of files but i think it's probably better for code health to do that. We don't want to be confused about whether we're using old or new values. We want to know it's legacy when we see it. I'd rather this const just not exist.
There was a problem hiding this comment.
So Constants.DefaultConcurrentTransfersCount is no longer being referenced at all, we are replacing all usage with the new Constants.LegacyDefaultConcurrentTransfersCount. The reason why we hid it rather than removed it is to avoid a breaking change
jaschrep-msft
left a comment
jaschrep-msft
left a comment
There was a problem hiding this comment.
nitpick suggestions. looks fine otherwise.
| - Added support for Server-side Encryption Rekeying. | ||
| - Added cross-tenant support for Principal-Bound User Delegation SAS. | ||
| - Added support for Dynamic User Delegation SAS. | ||
| - Updated the default concurrency transfer count. |
There was a problem hiding this comment.
I think this should go in an "Other changes" section and I think we should be more descriptive as this is a potentially impactful change. Something like:
"Changed the default "whatever the public facing setting is called" from 5 to "insert formula". This controls the maximum number of concurrent tasks that will be used during large uploads and downloads, and this change should result in higher throughput for these operations by default in most environments. This can be overridden by adjusting "public facing setting" or using the new compat switch "list compat switch and env var""
(I don't really know how compat switches work so write whatever makes sense, so customers know how to disable this. I think it's an app config and an env var)
Additionally, this change should affect more than just Blob, right? We should add this to any package is affects.
There was a problem hiding this comment.
We have this in the commons changelog as well. Will revise the wording to be more descriptive tho
There was a problem hiding this comment.
FYI the concurrency change seems to only affect partitionDownload and partitionedUpload. partitionDownload seems to be only for blobs and partitionedUpload is for blobs, datalake, and files it seems. Which is why I updated in both Blobs and Commons changelog
3 participants
Updated the default concurrency count. Also included compat switch logic to toggle back to previous default.