-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Added support to specify block size in BlobInputStream #15496
Added support to specify block size in BlobInputStream #15496
Conversation
@alzimmermsft Could you take a look at this PR for API feedback? |
BlobRange range = options.getRange() == null ? new BlobRange(0) : options.getRange(); | ||
int chunkSize = options.getBlockSize() == null ? 4 * Constants.MB : options.getBlockSize(); | ||
|
||
return new BlobInputStream(client, range.getOffset(), range.getCount(), chunkSize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a minor change where we will make an additional service call to get the properties of the blob before opening the stream. Are we okay with that? If so, we should add a quick document stating that we will be doing this as it is a behavioral change from the past.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think we already called getProperties before, I just moved it outside since we can't call it in the constructor before a call to super
/** | ||
* @return The size of each data chunk returned from the service. If block size is large, input stream will make | ||
* fewer network calls, but each individual call will send more data and will therefore take longer. | ||
* The default value is 4 MB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to say the default if 4 MB
should the getter check for blockSize == null
and return 4 * Constants.MB
?
return (blockSize == null) ? 4 * Constants.MB : blockSize;
If this change is made, we could also change the return type to int
instead of Integer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we tie this type to promising that? I'm a little hesitant to do that. I'd prefer it just be normal fluent behavior
[DO NOT MERGE]Dev resourcemover microsoft.migrate 2021 08 01 (Azure#15496) * Adds base for updating Microsoft.Migrate from version stable/2021-01-01 to version 2021-08-01 * Updates readme * Updates API version in new specs and examples * Added systemData property in Move Collection & Move Resource object * Update resourcemovercollection.json Added Tags and User-Managed Identities Properties * Update resourcemovercollection.json Made changes for removing tags property for document resources and resource group. * Removed additional empty line * Swagger PrettierCheck * Updating examples for 2021-08-01 version. * CI Fix Co-authored-by: Anany Shah <anashah@microsoft.com> Co-authored-by: yashjain4 <87814230+yashjain4@users.noreply.github.com>
[DO NOT MERGE]Dev resourcemover microsoft.migrate 2021 08 01 (Azure#15496) * Adds base for updating Microsoft.Migrate from version stable/2021-01-01 to version 2021-08-01 * Updates readme * Updates API version in new specs and examples * Added systemData property in Move Collection & Move Resource object * Update resourcemovercollection.json Added Tags and User-Managed Identities Properties * Update resourcemovercollection.json Made changes for removing tags property for document resources and resource group. * Removed additional empty line * Swagger PrettierCheck * Updating examples for 2021-08-01 version. * CI Fix Co-authored-by: Anany Shah <anashah@microsoft.com> Co-authored-by: yashjain4 <87814230+yashjain4@users.noreply.github.com>
Resolves #14739