-
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
Azure Storage Blob - BlockBlobClient Sync Stack Integration #34409
base: main
Are you sure you want to change the base?
Conversation
g2vinay
commented
Apr 11, 2023
•
edited
Loading
edited
- Adds Sync Stack to Storage Block Blob Client.
- Adds Sync logic to Storage Policies.
- Adds Missing Async tests for Storage Block Blob Client.
API change check APIView has identified API level changes in this PR and created following API reviews. |
.../azure-storage-blob/src/test/java/com/azure/storage/blob/specialized/BlockBlobAPITest.groovy
Outdated
Show resolved
Hide resolved
.../azure-storage-blob/src/test/java/com/azure/storage/blob/specialized/BlockBlobAPITest.groovy
Outdated
Show resolved
Hide resolved
Remaining Block Blob Client Missing Async tests to follow in upcoming commit this week. |
sdk/storage/azure-storage-perf/src/main/java/com/azure/storage/App.java
Outdated
Show resolved
Hide resolved
try { | ||
return blockWithOptionalTimeout(upload, timeout); | ||
return timeout != null | ||
? THREAD_POOL.submit(() -> operation.get()).get(timeout.toMillis(), TimeUnit.MILLISECONDS) | ||
: operation.get(); | ||
} catch (UncheckedIOException e) { | ||
throw LOGGER.logExceptionAsError(e); | ||
} catch (ExecutionException | TimeoutException | InterruptedException e) { | ||
throw LOGGER.logExceptionAsError(new RuntimeException(e)); | ||
} | ||
} |
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.
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.
+1
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.
other changes lgtm.
try { | ||
return blockWithOptionalTimeout(upload, timeout); | ||
return timeout != null | ||
? THREAD_POOL.submit(() -> operation.get()).get(timeout.toMillis(), TimeUnit.MILLISECONDS) | ||
: operation.get(); | ||
} catch (UncheckedIOException e) { | ||
throw LOGGER.logExceptionAsError(e); | ||
} catch (ExecutionException | TimeoutException | InterruptedException e) { | ||
throw LOGGER.logExceptionAsError(new RuntimeException(e)); | ||
} | ||
} |
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.
+1
BinaryData data = options.getData(); | ||
if (data == null) { | ||
if (options.getDataStream() != null) { | ||
data = BinaryData.fromStream(options.getDataStream()); |
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.
I wonder if this issue applies here too #33256
We should add a test case to confirm this is not an issue.
} | ||
|
||
// TODO (kasobol-msft) add metadata back (https://github.com/Azure/azure-sdk-for-net/issues/15969) |
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.
Todo can be removed, @ibrahimrabab ?
Hi @g2vinay. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
Hi @g2vinay. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing |
Reactivating just to get CI run coverage (feel free to close again if it's not something we want!) |