Storage queue migration by sarangan12 · Pull Request #25078 · Azure/azure-sdk-for-js

sarangan12 · 2023-03-03T06:42:01Z

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR:***(Applicable only to SDK release request PRs)*

Checklists

Added impacted package name to the issue description
Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
Added a changelog (if necessary)

sarangan12 · 2023-03-03T06:50:24Z

/azp run js - storage-queue - tests

azure-pipelines · 2023-03-03T06:50:36Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-sdk · 2023-03-03T07:01:56Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-storage-queue

sarangan12 · 2023-03-03T21:49:51Z

/azp run js - storage-queue - tests

azure-pipelines · 2023-03-03T21:50:05Z

Azure Pipelines failed to run 1 pipeline(s).

jeremymeng · 2023-03-03T21:59:52Z

sdk/storage/storage-queue/package.json

    "@azure/logger": "^1.0.0",
-    "tslib": "^2.2.0"
+    "tslib": "^2.2.0",
+    "@azure/storage-blob": "^12.20.0"


storage-queue is a much smaller package so I'd prefer not depending on storage-blob. @azure/storage-common approach would be better. Before that could we keep the copy of code?

file-datalake is different in that it actually uses blob client underneath for some operations.

@jeremymeng is the idea that we put the common files here and then reference them out of the tree? https://github.com/Azure/azure-sdk-for-js/tree/main/sdk/storage/storage-common/src

I wonder if it might also be worth converting storage-common to being a full package like we have done for keyvault-common recently. This would make it much easier to share code between storage libraries in the future. @EmmaZhu thoughts?

is the idea that we put the common files here and then reference them out of the tree?

no the full published common package is what I meant

For now I think we can have a copy in storage-queue.

Hm yeah seems like storage-file-share is in the same boat (no direct dep on storage-blob) so having a better story here seems important.

Having a storage-common package would have some benefit for the duplicated policies, it may also bring a long dependency chain and cause a bit higher dependency complexity in customer environment. I'll need to think it through more clearly. I don't expect it as a blocking for migration. If so, maybe we can do the change later to make the change more clean and easy to understand.

@EmmaZhu I know we have done the shared source code in the past, but the approach has its own problems and we believe common package approach is better for sharing. That's why we are now moving to use published common package instead (for example, keyvault-common, communication-common, identity is moving to use identity-common).

Yes I agree that separate PRs for migration and moving to common package are better for separate topics and make review easier

@jeremymeng , I agree that common package approach would bring more benefits. Let's have another PR for storage-common and maybe we should test some minor case like: customer is using storage-blob and storage-queue which references to different dependencies. Currently storage-blob and storage-queue already have some same dependencies, this should not bring any new potential issue, just we'd need to test the behavior.

jeremymeng · 2023-03-03T22:04:18Z

sdk/storage/storage-queue/review/storage-queue.api.md

-        parsedHeaders: MessageIdDeleteHeaders;
-    };
-};
+export type MessageIdDeleteResponse = MessageIdDeleteHeaders;


should these be WithResponse<> similar to what's in storage-blob

EmmaZhu · 2023-03-24T03:09:45Z

sdk/storage/storage-common/src/StorageRetryPolicyFactory.ts

+   * Storage server's default timeout policy will be used.
   *
-   * @see https://docs.microsoft.com/en-us/rest/api/storageservices/setting-timeouts-for-queue-service-operations
+   * @see https://docs.microsoft.com/en-us/rest/api/storageservices/setting-timeouts-for-blob-service-operations


The option is used by multiple services, but comments only mentioned blob.

EmmaZhu · 2023-03-24T03:10:23Z

sdk/storage/storage-common/src/credentials/AnonymousCredential.ts

 /**
- * AnonymousCredential provides a {@link CredentialPolicyCreator} member used to create
- * {@link AnonymousCredentialPolicy} objects. {@link AnonymousCredentialPolicy} is used with
+ * AnonymousCredential provides a credentialPolicyCreator member used to create


Why remove the link here?

EmmaZhu · 2023-03-24T03:18:57Z

sdk/storage/storage-common/src/policies/StorageRetryPolicy.ts

  retryPolicyType: StorageRetryPolicyType.EXPONENTIAL,
  secondaryHost: "",
-  tryTimeoutInMs: 30 * 1000, // https://docs.microsoft.com/en-us/rest/api/storageservices/setting-timeouts-for-queue-service-operations
+  tryTimeoutInMs: undefined, // Use server side default timeout strategy


Technically, it's a behavior change. Please keep the origin behavior.

EmmaZhu · 2023-03-24T03:21:35Z

sdk/storage/storage-queue/test/retrypolicy.spec.ts

@@ -1,158 +0,0 @@
-// Copyright (c) Microsoft Corporation.
-// Licensed under the MIT license.


Why remove this test suite?

github-actions · 2023-06-02T05:03:34Z

Hi @sarangan12. 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.

github-actions · 2023-06-09T08:34:51Z

Hi @sarangan12. 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 /reopen if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the no-recent-activity label; otherwise, this is likely to be closed again with the next cleanup pass.

xirzec · 2023-06-14T22:23:43Z

Superseded by #26207

sarangan12 added 8 commits March 2, 2023 11:34

Update README to a version of Autorest Typescript

1791fa4

Generated Code Changes

ef89499

Manual Changes in the Generated Code

fcb4b61

Code Changes for Migration to core-rest-pipeline

996d0a7

Code Changes II for Core Migration

0501096

Some File Deletions

45cdd6f

Update Package JSON File

4c6d684

Update Lock File

7adc55c

sarangan12 requested review from EmmaZhu, XiaoningLiu, bterlson, ckairen, deyaaeldeen, jeremymeng, jiacfan, ljian3377, vinjiang and xirzec as code owners March 3, 2023 06:42

ghost added the Storage Storage Service (Queues, Blobs, Files) label Mar 3, 2023

Merge branch 'main' into StorageQueueMigration

844daac

sarangan12 added 3 commits March 3, 2023 13:40

Fix Testing Issue I

4d3f8ec

Merge branch 'main' into StorageQueueMigration

9786124

Update Lock File

7563816

jeremymeng reviewed Mar 3, 2023

View reviewed changes

Create new package storage-common

6b1ec85

Update storage-blob to use storage-common

f4d5855

sarangan12 requested review from mikeharder and witemple-msft as code owners March 16, 2023 18:07

Update Pipeline

febb1cd

EmmaZhu reviewed Mar 24, 2023

View reviewed changes

github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Jun 2, 2023

github-actions bot closed this Jun 9, 2023

jeremymeng reopened this Jun 9, 2023

github-actions bot removed the no-recent-activity There has been no recent activity on this issue. label Jun 9, 2023

xirzec closed this Jun 14, 2023

		@@ -1,158 +0,0 @@
		// Copyright (c) Microsoft Corporation.
		// Licensed under the MIT license.

Conversation

sarangan12 commented Mar 3, 2023

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

Uh oh!

sarangan12 commented Mar 3, 2023

Uh oh!

azure-pipelines bot commented Mar 3, 2023

Uh oh!

azure-sdk commented Mar 3, 2023

Uh oh!

sarangan12 commented Mar 3, 2023

Uh oh!

azure-pipelines bot commented Mar 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremymeng Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 2, 2023

Uh oh!

github-actions bot commented Jun 9, 2023

Uh oh!

xirzec commented Jun 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Command used to generate this PR:***(Applicable only to SDK release request PRs)*

jeremymeng Mar 6, 2023 •

edited

Loading