Skip to content
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

[core-lro] No polling allowed when the operation is in a terminal state #24159

Merged

Conversation

deyaaeldeen
Copy link
Member

Packages impacted by this PR

@azure/core-lro

Issues associated with this PR

Failures in Azure/autorest.typescript#1586

Describe the problem that is addressed by this PR

poll() always sends the polling request as long as the operation has a location to poll from. However, once the operation is in a terminal state, there is no need to send the polling request.

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?

Sending the useless polling request is not the end of the world and won't affect the state of the poller. I don't feel strongly about the change but it is nice to not do unjustified extra work.

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

Yes.

Provide a list of related PRs (if any)

N/A

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)

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

Copy link
Contributor

@MaryGao MaryGao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use the version to test if it could pass the autores.typescript ci - Azure/autorest.typescript#1586?

@deyaaeldeen
Copy link
Member Author

Could you use the version to test if it could pass the autores.typescript ci - Azure/autorest.typescript#1586?

I verified that integration tests for both RLC and HLC pass with this version.

@deyaaeldeen deyaaeldeen enabled auto-merge (squash) January 5, 2023 05:06
Copy link
Contributor

@MaryGao MaryGao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@deyaaeldeen deyaaeldeen merged commit b0d5c13 into Azure:main Jan 5, 2023
@deyaaeldeen deyaaeldeen deleted the core-lro/no-polling-in-terminal-state branch January 5, 2023 06:00
deyaaeldeen added a commit that referenced this pull request Jan 11, 2023
This release contains the work in #24159 and is needed for the code generator tests to pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants