Batching in Azure Storage Blobs #5693

alzimmermsft · 2019-10-04T16:52:23Z

Adds batching into Azure Storage Blobs and implements the batching APIs for Blob delete and setTier.

pom.client.xml

JonathanGiles · 2019-10-07T00:29:43Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobBatch.java

+import java.util.function.Consumer;
+
+/**
+ * This class provides


...no javadoc

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobBatch.java

JonathanGiles · 2019-10-07T00:34:42Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobBatch.java

+     */
+    public Response<Void> delete(String containerName, String blobName) {
+        return deleteHelper(buildClient(containerName, blobName), null, null);
+    }


If this adds an operation (but doesn't actually perform it), it seems weird that this returns Response<Void>?

The planned implementation is a little round about where the submission will return a response that becomes populated once the batch submission returns. Could change the response type to a Future to better indicate that this doesn't do anything yet and document that the batch must be submitted via submitBatch before the response will have a value.

I think Mono instead of Future would be preferable so we don't introduce a separate paradigm just for this. It might be ok to have this as an "async only" feature where sync customers who want to use it just have to block because I agree that there should be some sort of type-enforced means of communicating that the return value isn't valid until the batch completes.

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobBatch.java

JonathanGiles · 2019-10-07T00:51:46Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobServiceAsyncClient.java

+     *
+     * <p>If any request in a batch fails this will throw a {@link StorageException}.</p>
+     *
+     * <p><strong>Code Samples</strong></p>


Need code samples

JonathanGiles · 2019-10-07T00:52:09Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobServiceAsyncClient.java

+     * <p><strong>Code Samples</strong></p>
+     *
+     * @param batch Batch to submit.
+     * @return An empty response indicating that the batch operation has completed.


Document how the user gets the response information for each operation.

JonathanGiles · 2019-10-07T00:55:29Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobServiceClient.java

+     *
+     * <p>If any request in a batch fails this will throw a {@link StorageException}.</p>
+     *
+     * <p><strong>Code Samples</strong></p>


rickle-msft · 2019-10-07T22:42:32Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobBatch.java

+            EXPECTED_SET_TIER_STATUS_CODES);
+    }
+
+    private <T> Response<T> createAndReturnBatchOperation(Mono<Response<T>> response, int... expectedStatusCodes) {


Why do we have to include expectedStatusCodes? Does that not get processed in RestProxy anymore in batch?

rickle-msft · 2019-10-08T17:48:31Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobBatch.java

+            .flatMap(batchOperation -> batchOperation)
+            .subscribe();
+
+        while (!disposable.isDisposed()) {


Why not flux.blockLast?

rickle-msft · 2019-10-08T17:50:27Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobBatch.java

+        }
+
+        Disposable disposable = Flux.fromStream(batchOperationQueue.stream())
+            .flatMap(batchOperation -> batchOperation)


Can you leave an inline comment that mentions the flatmap is just to force subscription on each of the inner requests? Otherwise it comes across as a bit cryptic to just map something to itself

rickle-msft · 2019-10-08T17:51:29Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobBatch.java

+     * and it adds the header "Content-Id" that allows the request to be mapped to the response.
+     */
+    private Mono<HttpResponse> cleanseHeaders(HttpPipelineCallContext context, HttpPipelineNextPolicy next) {
+        // Remove the "x-ms-header" as it shouldn't be included in the batch operation request.


"x-ms-version"

rickle-msft · 2019-10-08T19:33:02Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobBatch.java

+    }
+
+    String getContentType() {
+        return String.format("multipart/mixed; boundary=%s", batchBoundary);


Maybe this should be gathered with the other constants at the top?

rickle-msft · 2019-10-08T19:33:39Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobBatch.java

+        batchRequest.add(ByteBuffer.wrap(batchRequestBuilder.toString().getBytes(StandardCharsets.UTF_8)));
+        batchMapping.get(contentId).setRequest(request);
+
+        return Mono.empty();


Perhaps a Mono is more appropriate?

tg-msft

This looks great Alan! I added a couple of comments and questions, but none of it absolutely has to be addressed before checking in.

tg-msft · 2019-10-09T07:17:00Z

sdk/storage/azure-storage-blob-batch/README.md

+Azure Blob storage is Microsoft's object storage solution for the cloud. Blob
+storage is optimized for storing massive amounts of unstructured data.
+Unstructured data is data that does not adhere to a particular data model or
+definition, such as text or binary data.


Nit - add something about batching. I have:

Azure Blob storage is Microsoft's object storage solution for the cloud. Blob
storage is optimized for storing massive amounts of unstructured data. This
library allows you to batch multiple Azure Blob Storage operations in a single request.

I suppose this whole file could use some more Batch specifics. 😄

tg-msft · 2019-10-09T07:21:36Z

sdk/storage/azure-storage-blob-batch/src/main/java/com/azure/storage/blob/batch/BlobBatch.java

+     *
+     * @param client {@link BlobServiceClient} used to construct the batch.
+     */
+    public BlobBatch(BlobServiceClient client) {


Nit - if you're adding BlobBatchClient, then maybe that should be taken here.

tg-msft · 2019-10-09T07:24:42Z

sdk/storage/azure-storage-blob-batch/src/main/java/com/azure/storage/blob/batch/BlobBatch.java

+                batchPipelineBuilder.policies(this::cleanseHeaders);
+            }
+
+            batchPipelineBuilder.policies(pipeline.getPolicy(i));


Nit - just pass policy

tg-msft · 2019-10-09T07:27:47Z

sdk/storage/azure-storage-blob-batch/src/main/java/com/azure/storage/blob/batch/BlobBatch.java

+            .pipeline(pipeline)
+            .buildAsyncClient();
+
+        this.batchOperationQueue = new ConcurrentLinkedDeque<>();


Does this need to be Concurrent? In .NET our clients are thread safe but our models aren't. You need to synchronize access to the model if you want to share it across threads, but I'm not going to impose that cost on everyone else. BlobBatch looks like a client, but it's really a model type.

I wouldn't bother changing this before you ship v1.

tg-msft · 2019-10-09T07:29:12Z

sdk/storage/azure-storage-blob-batch/src/main/java/com/azure/storage/blob/batch/BlobBatch.java

+     * @throws UnsupportedOperationException If this batch has already added an operation of another type.
+     */
+    public Response<Void> delete(String blobUrl) {
+        return deleteHelper(getUrlPath(blobUrl), null, null);


Is it just getting the path? Not the query as well? That might be okay because I guess your SAS is handled with a credential instead of being passed around on the URI.

tg-msft · 2019-10-09T07:52:34Z

...age/azure-storage-blob-batch/src/main/java/com/azure/storage/blob/batch/BlobBatchHelper.java

+                    setStatusCodeAndHeaders(batchOperationResponse, subResponseSections[1]);
+
+                    // The third section will contain the body.
+                    if (subResponseSections.length > 2) {


tg-msft · 2019-10-09T07:54:42Z

...age/azure-storage-blob-batch/src/main/java/com/azure/storage/blob/batch/BlobBatchHelper.java

+                continue;
+            }
+
+            if (line.startsWith("HTTP")) {


Nit - should we just add a Boolean first = true outside the loop and then reset it here? What if you get a header starting with HTTP?

tg-msft · 2019-10-09T07:56:06Z

...torage-blob-batch/src/main/java/com/azure/storage/blob/batch/BlobBatchOperationResponse.java

+ *
+ * @param <T> The deserialized type of the response content, available from {@link #getValue()}.
+ */
+public class BlobBatchOperationResponse<T> implements Response<T> {


Should this be public? in .NET we've made ours private and users only publicly see Response<T>.

tg-msft · 2019-10-09T08:00:20Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobServiceAsyncClient.java

@@ -3,48 +3,45 @@

 package com.azure.storage.blob;



Is this autogenerated? Why the massive diff delta?

tg-msft · 2019-10-09T08:01:20Z

...age/azure-storage-blob/src/main/java/com/azure/storage/blob/implementation/ServicesImpl.java

@@ -97,7 +98,7 @@ public ServicesImpl(AzureBlobStorageImpl client) {
        Mono<ServicesGetAccountInfoResponse> getAccountInfo(@HostParam("url") String url, @HeaderParam("x-ms-version") String version, @QueryParam("restype") String restype, @QueryParam("comp") String comp, Context context);

        @Post("")
-        @ExpectedResponses({200})
+        @ExpectedResponses({202})


Did you add a readme transform? Is it in your PR?

joshfree · 2019-10-09T17:48:49Z

sdk/core/azure-core/src/main/java/module-info.java

@@ -73,6 +73,7 @@
        com.azure.security.keyvault.secrets,    // FIXME this should not be a long-term solution
        com.azure.storage.common,               // FIXME this should not be a long-term solution
        com.azure.storage.blob,                 // FIXME this should not be a long-term solution
+        com.azure.storage.blob.batch,           // FIXME this should not be a long-term solution


can you file a tracking issue for all of these FIXMEs

joshfree · 2019-10-09T17:49:45Z

sdk/storage/azure-storage-blob-batch/README.md

+Storage Blob to use Netty HTTP client. 
+
+### Alternate HTTP client
+If, instead of Netty it is preferable to use OkHTTP, there is a HTTP client available for that too. Exclude the default


nit: remove comma after If

joshfree · 2019-10-09T17:57:53Z

...orage-blob/src/samples/java/com/azure/storage/blob/BlobServiceClientJavaDocCodeSnippets.java

@@ -240,4 +246,48 @@ public void getAccountInfoWithResponse() {
        StorageAccountInfo accountInfo = client.getAccountInfoWithResponse(timeout, context).getValue();
        // END: com.azure.storage.blob.BlobServiceClient.getAccountInfoWithResponse#Duration-Context
    }
+


It looks like all of our batching tests are batching just 2 operations. Can we test with an N greater than 2 as well?

alzimmermsft · 2019-10-09T21:01:54Z

Closed in favor of #5734

alzimmermsft added 30 commits September 13, 2019 10:58

Clients and code samples for LeaseClients

bc7f532

Updated tests

99948a4

Updating unit tests

4b13ea4

Using builder pattern for LeaseClients

b36de0d

Merge branch 'master' into AzStorage_LeaseClient

eeec369

Fix unit tests

0ba893a

Cleaning up documentation

52f0a11

Fix linting issues and more unit tests

52f9134

Merge branch 'AzStorage_LeaseClient' into AzStorage_BatchClient

fb1d02a

Initial classes

549856a

Merge branch 'master' into AzStorage_LeaseClient

1b18093

Some moving of clients

dcd005c

Moved test files into correct folder

65ea68c

Merge branch 'AzStorage_LeaseClient' into AzStorage_MoveSubBlobs

28d6e77

Moved sub-blob clients to specialized namespace, added BlobBaseClients

a91f4ae

Refactoring unit tests

d77b4cd

More cleaning up

63f0e1b

More cleaning up

a619012

Merged in master

2f595bb

Cleaning up linting issues

51da041

Addressing PR feedback

307fd0d

Merged in master

10d8ca6

Merge branch 'AzStorage_MoveSubBlobs' into AzStorage_BatchClient

f43b090

More implementation

c1c02d0

More implementation for batching

389ba54

Merged in master

a49a98a

Merge branch 'AzStorage_MoveSubBlobs' into AzStorage_BatchClient

4a15b54

Merged in master

d300657

Merge branch 'master' into AzStorage_MoveSubBlobs

cc60e01

Cleaning up Spotbug and linting issues

8919c8c

alzimmermsft added 3 commits October 4, 2019 15:43

Merged in master

e19f5de

Merge branch 'AzCore_RemoveHeader' into AzStorage_BatchClient

be73cee

Update unit tests, adding more documentation of implementation

8d8fb4a

alzimmermsft marked this pull request as ready for review October 5, 2019 04:07

alzimmermsft requested review from gapra-msft, hemanttanwar, jaschrep-msft, jianghaolu, rickle-msft and sima-zhu as code owners October 5, 2019 04:07

JonathanGiles reviewed Oct 7, 2019

View reviewed changes

alzimmermsft added 10 commits October 7, 2019 14:21

Merged in master

90e0c3c

Revert POM changes

a1dac85

Cleaning up implementation

aa57489

Using anonymous classes now instead of implemented private classes

f9d7452

Build and checkstyle cleanup, initial samples

59bcb01

Adding code samples

495d840

Move batching into a seperate module

260bf6e

Cleanup change log

47f997c

Merge branch 'master' into AzStorage_BatchClient

dc53444

Reverting some changes in branch

fa88d61

rickle-msft reviewed Oct 8, 2019

View reviewed changes

tg-msft approved these changes Oct 9, 2019

View reviewed changes

joshfree reviewed Oct 9, 2019

View reviewed changes

joshfree approved these changes Oct 9, 2019

View reviewed changes

alzimmermsft closed this Oct 9, 2019

alzimmermsft deleted the AzStorage_BatchClient branch March 20, 2020 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batching in Azure Storage Blobs #5693

Batching in Azure Storage Blobs #5693

alzimmermsft commented Oct 4, 2019

JonathanGiles Oct 7, 2019

JonathanGiles Oct 7, 2019

alzimmermsft Oct 7, 2019

rickle-msft Oct 7, 2019

JonathanGiles Oct 7, 2019

JonathanGiles Oct 7, 2019

JonathanGiles Oct 7, 2019

rickle-msft Oct 7, 2019

rickle-msft Oct 8, 2019

rickle-msft Oct 8, 2019

rickle-msft Oct 8, 2019

rickle-msft Oct 8, 2019

rickle-msft Oct 8, 2019

tg-msft left a comment

tg-msft Oct 9, 2019

tg-msft Oct 9, 2019

tg-msft Oct 9, 2019

tg-msft Oct 9, 2019

tg-msft Oct 9, 2019

tg-msft Oct 9, 2019

tg-msft Oct 9, 2019

tg-msft Oct 9, 2019

tg-msft Oct 9, 2019

tg-msft Oct 9, 2019

tg-msft Oct 9, 2019

joshfree Oct 9, 2019

joshfree Oct 9, 2019

joshfree Oct 9, 2019

alzimmermsft commented Oct 9, 2019

Batching in Azure Storage Blobs #5693

Batching in Azure Storage Blobs #5693

Conversation

alzimmermsft commented Oct 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tg-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alzimmermsft commented Oct 9, 2019