Storage: Updated Listing Operation Return Types #4803

jaschrep-msft · 2019-08-02T00:07:27Z

Storage SDK upated to new listing API standards. (Not yet paged listing
for sync APIs)
Async listing return types updated from Flux to PagedFlux
Sync listing return types updated from Iterable to Stream

Bug Fixes:
Fixed bug where Flux<PagedResponse> returned from PagedFlux::byPage()
would attempt to follow non-existent continuation tokens.

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

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

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerAPITest.groovy

sima-zhu · 2019-08-02T18:47:41Z

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

        Flux<ContainerItem> response = blobServiceAsyncClient.listContainers(options);

-        return timeout == null ? response.toIterable() : response.timeout(timeout).toIterable();
+        return timeout == null ? response.toStream() : response.timeout(timeout).toStream();


Alan's PR has a helper method for the timeout in Utility. Is that visible to you?

That method is only for Mono responses, this is a Flux.

sima-zhu · 2019-08-02T18:52:24Z

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

-            });
+
+        return new PagedFlux<>(
+            () -> postProcessResponse(this.azureBlobStorage.blockBlobs().getBlockListWithRestResponseAsync(


It is too complex to understand. Could you please have a helper method?

sima-zhu · 2019-08-02T18:53:56Z

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

+                        .collect(Collectors.toList()),
+                    null /* nextLink */,
+                    response.deserializedHeaders())),
+            (marker) -> null);


Why is there no next page? If there is a justification, could you have it in Javadoc?

This is a single response Flux, this is a current gap in our Flux and PagedFlux returns as it doesn't fit either cleanly. This will be a discussion for Preview 3

If other list APIs have a way to do pagination, then customer would expect this one has pagination as well. It is better to have javadoc to make it clear.

sima-zhu · 2019-08-02T18:56:02Z

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

+        final BlobAccessConditions finalAccessConditions = accessConditions == null ? new BlobAccessConditions() : accessConditions;
+
+        return new PagedFlux<>(
+            () -> postProcessResponse(this.azureBlobStorage.pageBlobs().getPageRangesWithRestResponseAsync(


Can you have a local variable for the postResponse? Too complex to digest.

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

JonathanGiles · 2019-08-13T02:26:31Z

For the sync case, the current plan is, rather than return Iterable or Stream, to return IterableResponse or PagedIterable, which are both in azure-core now. Please take a look and consider making use of these.

…into OLD-paged-flux

jaschrep-msft · 2019-08-22T21:22:04Z

Updated sync clients to use PagedIterable<T>. Added overloads to PagedIterable<T> to bring it up to speed with PagedFlux<T> functionality.

sdk/core/azure-core/src/main/java/com/azure/core/http/rest/PagedIterable.java

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerAPITest.groovy

gapra-msft

Made some comments about commented out tests

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

rickle-msft · 2019-08-23T13:00:06Z

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

@@ -379,19 +377,12 @@ private String getBlockID() {
     *
     * @return A reactive response containing the list of blocks.
     */
-    public Flux<BlockItem> listBlocks(BlockListType listType,
+    public Mono<Response<BlockList>> listBlocks(BlockListType listType,


Did we have a conversation with Jonathan about non-paged listing operations returning Mono? It seems to me that Flux is more natural, but if that discussion has been had, then I'll go along with it.

@JonathanGiles has there been a decision on return types for listing operations that aren't paginated?

Tracked separately under #5097

Yeah - this change isn't what I expected to see either. I would have preferred to see a PagedFlux here, even though there is only one page. I'll clarify the spec on this this week. I also suspect you would like to see improvements to the PagedFlux constructors for situations where there is no follow-up pages in the response - please do feel free to submit PRs to improve that (or else let me know and I will ensure it gets improved).

rickle-msft · 2019-08-23T13:06:37Z

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

@@ -707,111 +760,19 @@ public URL getContainerUrl() {
     * [!code-java[Sample_Code](../azure-storage-java/src/test/java/com/microsoft/azure/storage/Samples.java?name=list_blobs_hierarchy_helper "helper code for ContainerAsyncClient.listBlobsHierarchySegment")] \n
     * For more samples, please see the [Samples file](%https://github.com/Azure/azure-storage-java/blob/master/src/test/java/com/microsoft/azure/storage/Samples.java)
     */
-    private Mono<ContainersListBlobHierarchySegmentResponse> listBlobsHierarchySegment(String marker, String delimiter, ListBlobsOptions options) {
+    private Mono<ContainersListBlobHierarchySegmentResponse> listBlobsHierarchySegment(String marker, String delimiter,
+            ListBlobsOptions options, Duration timeout) {


I don't think we generally accept timeout as a parameter on async apis. I'd also be surprised if we wanted this overload that returns ContainersListBlobHierarchySegmentResponse. I think we always want it to be PagedFlux, no?

This is a private helper method for the paginated methods above. The PagedFlux implementation makes use of this to lazily fetch the next page.

Regarding the timeout, this is because of the synchronous wrappers for the paginated method; none of the public async methods accept a timeout. The synchronous methods for paginated listings must wrap the PagedFlux in a PagedIterable. However, if the user specifies a timeout for this operation, we cannot apply a timeout on the PagedFlux BEFORE the user decides if they want this by-item or by-page (Flux::timeout(Duration) returns TimeoutFlux, a hidden implementation of Flux, destroying our ability to use PagedFlux functionality). Since we cannot apply the timeout at the syncrhonous convenience layer, we need to find a new place. The chosen method here is diving down and applying the timeout to each individual Mono in the lazily-fetched pages. Considering Flux::timeout(Duration) applies the timeout between each individual emission, this is nearly identical to how a timeout would be applied to the Flux as a whole.

rickle-msft · 2019-08-23T13:31:05Z

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/BlockBlobAPITest.groovy


        then:
        headers.value("x-ms-request-id") != null
        headers.value("x-ms-version") != null
        headers.value("x-ms-content-crc64") != null
        headers.value("x-ms-request-server-encrypted") != null

+        def response = bu2.listBlocks(BlockListType.ALL)
+        response.uncommittedBlocks().size() == 1


What was the conversation around spliting up these two lists? It seems like we'd want to be consistent with the fact that blobItems and prefixes are presented in one list

My understanding is this:

For listing blobs in a container, this is a paginated listing operation that returns two different lists, so auto-continuing two lists at the same time while someone is likely only consuming one at a time is a problematic approach. Therefore we join the lists and have a flag to tell you what is what. Since this API simulates listing blobs and "directories," this is actually useful, as the JDK provides similar functionality when browsing the local filesystem.

For listing blocks in a blob, this is not paginated. In the world where we are not using the PagedFlux listing approach for operations that are not paged, we are already returning a Mono of a buffered data structure, and so we can keep the two lists pre-separated for the user.

@JonathanGiles may have input on this.

Answer to this is likely dependent on outcome of #5097, which is tracked separately.

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerAPITest.groovy

rickle-msft

A few questions/thoughts

sima-zhu

Much clearer structure. Ship it

jaschrep-msft requested review from rickle-msft and alzimmermsft August 2, 2019 00:07

jaschrep-msft requested review from hemanttanwar, jianghaolu and srnagar as code owners August 2, 2019 00:07

jaschrep-msft added Storage Storage Service (Queues, Blobs, Files) Azure.Core azure-core labels Aug 2, 2019

jaschrep-msft requested a review from sima-zhu as a code owner August 2, 2019 16:40

alzimmermsft approved these changes Aug 2, 2019

View reviewed changes

sima-zhu reviewed Aug 2, 2019

View reviewed changes

jaschrep-msft added 3 commits August 19, 2019 08:43

Temp work

62e7d88

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

3fa61fb

…into OLD-paged-flux

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

dbf403a

…into OLD-paged-flux

jaschrep-msft force-pushed the storage-listing-pageflux branch from 87d8c41 to dbf403a Compare August 22, 2019 21:18

jaschrep-msft requested a review from gapra-msft as a code owner August 22, 2019 21:18

jaschrep-msft added 2 commits August 22, 2019 14:33

Fixed likely merge artifact causing infinite loop

469d7a4

Fixed Checkstyle Issues

9160521

jaschrep-msft requested a review from sima-zhu August 22, 2019 22:24

gapra-msft reviewed Aug 22, 2019

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/http/rest/PagedIterable.java Outdated Show resolved Hide resolved

gapra-msft reviewed Aug 22, 2019

View reviewed changes

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerAPITest.groovy Outdated Show resolved Hide resolved

gapra-msft reviewed Aug 22, 2019

View reviewed changes

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerAPITest.groovy Outdated Show resolved Hide resolved

gapra-msft reviewed Aug 22, 2019

View reviewed changes

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerAPITest.groovy Show resolved Hide resolved

gapra-msft requested changes Aug 22, 2019

View reviewed changes

Reactivated paged listing tests

688a410

rickle-msft reviewed Aug 23, 2019

View reviewed changes

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlockBlobAsyncClient.java Outdated Show resolved Hide resolved

rickle-msft reviewed Aug 23, 2019

View reviewed changes

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerAPITest.groovy Outdated Show resolved Hide resolved

rickle-msft reviewed Aug 23, 2019

View reviewed changes

sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerAPITest.groovy Outdated Show resolved Hide resolved

rickle-msft reviewed Aug 23, 2019

View reviewed changes

Comsetic PR Comments addressed

5c2798e

jaschrep-msft requested review from gapra-msft and rickle-msft August 23, 2019 18:12

gapra-msft approved these changes Aug 23, 2019

View reviewed changes

sima-zhu approved these changes Aug 23, 2019

View reviewed changes

jaschrep-msft merged commit 8bd0f47 into Azure:master Aug 23, 2019

jaschrep-msft deleted the storage-listing-pageflux branch August 23, 2019 20:46

JonathanGiles mentioned this pull request Sep 2, 2019

PagedIterable - should it take a continuation token for the *ByPage methods? #4989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage: Updated Listing Operation Return Types #4803

Storage: Updated Listing Operation Return Types #4803

jaschrep-msft commented Aug 2, 2019

sima-zhu Aug 2, 2019

alzimmermsft Aug 2, 2019

sima-zhu Aug 2, 2019

sima-zhu Aug 2, 2019

alzimmermsft Aug 2, 2019

sima-zhu Aug 2, 2019

sima-zhu Aug 2, 2019

JonathanGiles commented Aug 13, 2019

jaschrep-msft commented Aug 22, 2019

gapra-msft left a comment

rickle-msft Aug 23, 2019

jaschrep-msft Aug 23, 2019

jaschrep-msft Aug 23, 2019

JonathanGiles Aug 25, 2019

rickle-msft Aug 23, 2019

jaschrep-msft Aug 23, 2019

rickle-msft Aug 23, 2019

jaschrep-msft Aug 23, 2019

jaschrep-msft Aug 23, 2019

jaschrep-msft Aug 23, 2019 •

edited

Loading

rickle-msft left a comment

sima-zhu left a comment

Storage: Updated Listing Operation Return Types #4803

Storage: Updated Listing Operation Return Types #4803

Conversation

jaschrep-msft commented Aug 2, 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

JonathanGiles commented Aug 13, 2019

jaschrep-msft commented Aug 22, 2019

gapra-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

jaschrep-msft Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

rickle-msft left a comment

Choose a reason for hiding this comment

sima-zhu left a comment

Choose a reason for hiding this comment

jaschrep-msft Aug 23, 2019 •

edited

Loading