[storage][blob] Create BlobBatchClient based on API review feedback #5634

xirzec · 2019-10-17T21:16:22Z

This change:

Renames BatchRequest to BatchBlob
Flattens BatchDeleteRequest and BatchSetTierRequest into BatchBlob
Moves submitBatch code from BlobServiceClient into new BlobBatchClient
Created new deleteBlobs and setBlobsAccessTier helpers on BlobBatchClient
Adjusted existing tests
Added new tests for deleteBlobs and setBlobsAccessTier

sdk/storage/storage-blob/src/BlobBatchClient.ts

sdk/storage/storage-blob/test/blobbatch.spec.ts

jeremymeng · 2019-10-17T22:25:23Z

  .byPage({ maxPageSize: 1 })

Not sure why the old code retrieve a page with one item, we should remove the option. #Resolved

Refers to: sdk/storage/storage-blob/test/blobbatch.spec.ts:113 in cf04032. [](commit_id = cf04032, deletion_comment = False)

jeremymeng · 2019-10-17T22:26:11Z

  .byPage({ maxPageSize: 1 })

oh, possibly just like .Any()

In reply to: 543386750 [](ancestors = 543386750)

Refers to: sdk/storage/storage-blob/test/blobbatch.spec.ts:113 in cf04032. [](commit_id = cf04032, deletion_comment = False)

jeremymeng

jiacfan · 2019-10-18T01:46:31Z

Add a delete operation(subrequest) to mark the specified blob or snapshot for deletion.

Same comments for deleteBlob and setBlobTierAccess APIs, please ensure the comments across languages, I think that would be important as well, as the document is exposed to users and it's their first guide.

Refers to: sdk/storage/storage-blob/src/BlobBatch.ts:123 in 42fee72. [](commit_id = 42fee72, deletion_comment = False)

jiacfan · 2019-10-18T01:47:57Z

sdk/storage/storage-blob/src/BlobServiceClient.ts

-   * ```
-   *
-   * @see https://docs.microsoft.com/en-us/rest/api/storageservices/blob-batch
+   * Creates a BlobBatchClient object


minor: missed . at end, and please ensure the comment alignment with other languages, e.g.: should here be link of introduction to BlobBatchClient object?

jiacfan · 2019-10-18T01:49:39Z

sdk/storage/storage-blob/src/index.browser.ts

@@ -12,7 +12,7 @@ export * from "./BlobClient";
 export * from "./AppendBlobClient";
 export * from "./BlockBlobClient";
 export * from "./PageBlobClient";
-export * from "./BatchRequest";
+export * from "./BlobBatch";


BlobBatchClient.ts is missed here?

jiacfan · 2019-10-18T01:49:51Z

sdk/storage/storage-blob/src/index.ts

@@ -16,7 +16,7 @@ export * from "./AccountSASPermissions";
 export * from "./AccountSASResourceTypes";
 export * from "./AccountSASServices";
 export * from "./AccountSASSignatureValues";
-export * from "./BatchRequest";
+export * from "./BlobBatch";


BlobBatchClient.ts is missed here?

sdk/storage/storage-blob/src/BlobBatchClient.ts

jiacfan

🕐

XiaoningLiu · 2019-10-18T03:36:58Z

A BatchRequest represents a based class for BatchDeleteRequest and BatchSetTierRequest.

typo

Refers to: sdk/storage/storage-blob/src/BlobBatch.ts:52 in 42fee72. [](commit_id = 42fee72, deletion_comment = False)

XiaoningLiu · 2019-10-18T03:37:46Z

sdk/storage/storage-blob/src/BlobBatch.ts

@@ -126,7 +113,7 @@ export class BatchDeleteRequest extends BatchRequest {
   * @returns {Promise<void>}
   * @memberof BatchDeleteRequest


BatchDeleteRequest [](start = 15, length = 18)

needs to update

XiaoningLiu · 2019-10-18T03:44:21Z

sdk/storage/storage-blob/src/BlobBatchClient.ts

+   * @param {BlobBatch} batchRequest A set of Delete or SetTier operations.
+   * @param {BlobBatchSubmitBatchOptionalParams} [options]
+   * @returns {Promise<BlobBatchSubmitBatchResponse>}
+   * @memberof BlobServiceClient


BlobServiceClient [](start = 15, length = 17)

typo, please check all inline Jsdoc comments align with new added classes

* Fix a number of comment issues * Prevent consumer from mixing operation types inside a single batch * Actually export the new types

xirzec · 2019-10-18T19:14:47Z

Same comments for deleteBlob and setBlobTierAccess APIs, please ensure the comments across languages, I think that would be important as well, as the document is exposed to users and it's their first guide.

I aligned the comments with .NET, which seems to have copied them verbatim from the REST documentation. Let me know if this is what you expected.

jiacfan

jiacfan · 2019-10-21T03:08:02Z

sdk/storage/storage-blob/src/BlobBatch.ts

@@ -49,14 +49,16 @@ export interface BatchSubRequest {
 }

 /**
- * A BatchRequest represents a based class for BatchDeleteRequest and BatchSetTierRequest.
+ * A BlobBatch represents an aggregated set of operations on blobs.
+ * Currently, only delete and setAccessTier are supported.


Would better add comments like: only one kind of operation is supported per batch. This might be applicable to other langs.

jiacfan · 2019-10-21T03:09:22Z

Please remember to test it with CI and live account testing, and test samples with import the package to ensure everything works.

XiaoningLiu · 2019-10-21T04:05:56Z

sdk/storage/storage-blob/src/BlobBatch.ts

+   * This operation does not update the blob's ETag.
+   * For detailed information about block blob level tiering
+   * see [hot, cool, and archive access tiers](https://docs.microsoft.com/en-us/azure/storage/blobs/storage-blob-storage-tiers).
+   * The operation will be authenticated and authorized
   * with specified credential.See [blob batch authorization details](https://docs.microsoft.com/en-us/rest/api/storageservices/blob-batch#authorization).
   *
   * @param {string} url The url of the blob resource to delete.
   * @param {Credential} credential The credential to be used for authentication and authorization.


SharedKeyCredential | AnonymousCredential | TokenCredential

Please go through types for JSDoc parameters

HarshaNalluru · 2019-10-21T18:37:22Z

/azp run js - storage-blob - tests

azure-pipelines · 2019-10-21T18:37:34Z

Azure Pipelines successfully started running 1 pipeline(s).

xirzec added 5 commits October 16, 2019 17:42

Refactor BatchRequest

91e4eb6

fixup

21d3c40

Rename BatchRequest to BlobBatch

d56b3d9

Move logic from BlobServiceClient to BlobBatchClient

a213fbb

Fix BlobBatch tests

cf04032

xirzec added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Oct 17, 2019

xirzec requested review from XiaoningLiu, jeremymeng and HarshaNalluru October 17, 2019 21:16

xirzec requested review from jiacfan and vinjiang as code owners October 17, 2019 21:16

xirzec self-assigned this Oct 17, 2019

Merge master

cf7cebd

HarshaNalluru reviewed Oct 17, 2019

View reviewed changes

sdk/storage/storage-blob/src/BlobBatchClient.ts Outdated Show resolved Hide resolved

Rename response types

e0499f4

joheredi reviewed Oct 17, 2019

View reviewed changes

sdk/storage/storage-blob/src/BlobBatchClient.ts Outdated Show resolved Hide resolved

jeremymeng reviewed Oct 17, 2019

View reviewed changes

sdk/storage/storage-blob/test/blobbatch.spec.ts Show resolved Hide resolved

Add tests for new methods

056c378

joheredi approved these changes Oct 17, 2019

View reviewed changes

Skip new tests in playback due to uuids

42fee72

HarshaNalluru approved these changes Oct 17, 2019

View reviewed changes

jeremymeng approved these changes Oct 17, 2019

View reviewed changes

jiacfan reviewed Oct 18, 2019

View reviewed changes

sdk/storage/storage-blob/src/BlobBatchClient.ts Show resolved Hide resolved

jiacfan requested changes Oct 18, 2019

View reviewed changes

XiaoningLiu reviewed Oct 18, 2019

View reviewed changes

xirzec added 2 commits October 18, 2019 11:55

Respond to PR feedback

82e2e9a

* Fix a number of comment issues * Prevent consumer from mixing operation types inside a single batch * Actually export the new types

Update comments

7c61f35

xirzec requested a review from jiacfan October 18, 2019 19:14

jiacfan approved these changes Oct 21, 2019

View reviewed changes

jiacfan reviewed Oct 21, 2019

View reviewed changes

XiaoningLiu reviewed Oct 21, 2019

View reviewed changes

XiaoningLiu approved these changes Oct 21, 2019

View reviewed changes

ramya-rao-a mentioned this pull request Oct 21, 2019

[Storage][Blob] explicitly re-export individual model types #5567

Merged

xirzec added 3 commits October 21, 2019 11:03

Merge master

538f15c

fixup

b5bbcc2

Address PR feedback

265e5bb

fix sample logging

bdeaf5a

xirzec merged commit 8c3d1b4 into Azure:master Oct 21, 2019

xirzec deleted the adjustBatchClient branch October 21, 2019 20:06

ramya-rao-a mentioned this pull request Oct 21, 2019

[Storage][Blob]Disable extract-api #5695

Merged

xirzec mentioned this pull request Oct 21, 2019

[storage][blob] Fix BlobBatchClient constructor to use public types #5699

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[storage][blob] Create BlobBatchClient based on API review feedback #5634

[storage][blob] Create BlobBatchClient based on API review feedback #5634

xirzec commented Oct 17, 2019 •

edited

jeremymeng commented Oct 17, 2019 •

edited

jeremymeng commented Oct 17, 2019

jeremymeng left a comment

jiacfan commented Oct 18, 2019

jiacfan Oct 18, 2019 •

edited

jiacfan Oct 18, 2019

jiacfan Oct 18, 2019

jiacfan left a comment

XiaoningLiu commented Oct 18, 2019

XiaoningLiu Oct 18, 2019

XiaoningLiu Oct 18, 2019

xirzec commented Oct 18, 2019

jiacfan left a comment

jiacfan Oct 21, 2019

jiacfan commented Oct 21, 2019

XiaoningLiu Oct 21, 2019

HarshaNalluru commented Oct 21, 2019

azure-pipelines bot commented Oct 21, 2019

		@@ -126,7 +113,7 @@ export class BatchDeleteRequest extends BatchRequest {
		* @returns {Promise<void>}
		* @memberof BatchDeleteRequest

[storage][blob] Create BlobBatchClient based on API review feedback #5634

[storage][blob] Create BlobBatchClient based on API review feedback #5634

Conversation

xirzec commented Oct 17, 2019 • edited

jeremymeng commented Oct 17, 2019 • edited

jeremymeng commented Oct 17, 2019

jeremymeng left a comment

Choose a reason for hiding this comment

jiacfan commented Oct 18, 2019

jiacfan Oct 18, 2019 • edited

Choose a reason for hiding this comment

jiacfan Oct 18, 2019

Choose a reason for hiding this comment

jiacfan Oct 18, 2019

Choose a reason for hiding this comment

jiacfan left a comment

Choose a reason for hiding this comment

XiaoningLiu commented Oct 18, 2019

XiaoningLiu Oct 18, 2019

Choose a reason for hiding this comment

XiaoningLiu Oct 18, 2019

Choose a reason for hiding this comment

xirzec commented Oct 18, 2019

jiacfan left a comment

Choose a reason for hiding this comment

jiacfan Oct 21, 2019

Choose a reason for hiding this comment

jiacfan commented Oct 21, 2019

XiaoningLiu Oct 21, 2019

Choose a reason for hiding this comment

HarshaNalluru commented Oct 21, 2019

azure-pipelines bot commented Oct 21, 2019

xirzec commented Oct 17, 2019 •

edited

jeremymeng commented Oct 17, 2019 •

edited

jiacfan Oct 18, 2019 •

edited