Remove getProperties from ShareFileClient.downloadToFile() #32112
Conversation
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
…to downloadToFileRemoveProperties
…to downloadToFileRemoveProperties
…to downloadToFileRemoveProperties
|
/azp run java - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run java - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run java - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run java - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| headers.getCopySource(), | ||
| headers.getCopyStatus(), | ||
| headers.isServerEncrypted(), | ||
| null); |
There was a problem hiding this comment.
One minor worry is customers who were using FileSmbProperties from download before will no longer have access to them.
| ShareFileRange finalRange = range == null ? new ShareFileRange(0) : range; | ||
| return downloadWithResponse(new ShareFileDownloadOptions().setRange(finalRange) | ||
| .setRequestConditions(requestConditions), context) | ||
| .subscribeOn(Schedulers.boundedElastic()) |
There was a problem hiding this comment.
nit: the subscribeOn isn't necessary
| private Mono<Response<ShareFileProperties>> downloadSingleChunk(AsynchronousFileChannel channel, ShareFileRange range, | ||
| ShareRequestConditions requestConditions, Context context) { | ||
| ShareFileRange finalRange = range == null ? new ShareFileRange(0) : range; | ||
| return downloadWithResponse(new ShareFileDownloadOptions().setRange(finalRange) |
There was a problem hiding this comment.
If possible, this should get wrapped with FluxUtil.createRetriableDownloadFlux so the download can be resumed from a midpoint if it fails.
| .subscribeOn(Schedulers.boundedElastic()) | ||
| .retryWhen(Retry.max(3).filter(throwable -> throwable instanceof IOException | ||
| || throwable instanceof TimeoutException)); |
There was a problem hiding this comment.
Similar comment where these aren't needed. We should let better retrying mechanisms handle this.
| || throwable instanceof TimeoutException)))) | ||
| .then(Mono.just(response)); | ||
|
|
||
| private static Mono<Void> writeBodyToFile(ShareFileDownloadAsyncResponse response, AsynchronousFileChannel channel, |
There was a problem hiding this comment.
We may want to expose this through a new API, similar to what Blobs is doing where the Response type is something custom. Callers into the download without Response can call through this as well. That way we can offer a better choice but retain functionality of ShareFileProperties, and we won't need to add the translation code.
|
@ibrahimrabab removing |
|
Closing this PR since it is not possible to extract FileSmbProperties from the downloadWithResponse call. getProperties() call is needed to ensure we extract all relevant information needed by the user. |
Description
resolves #4527
Previously ShareFileAsyncClient.downloadToFile() made an unnecessary call of .getProperties().To avoid this, we are downloading using a single call to downloadWithResponse().
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines