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

[KeyVault] Consider updating LRO implementations to use OperationInternal class #22912

Closed
kinelski opened this issue Jul 27, 2021 · 5 comments · Fixed by #25593
Closed

[KeyVault] Consider updating LRO implementations to use OperationInternal class #22912

kinelski opened this issue Jul 27, 2021 · 5 comments · Fixed by #25593
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault

Comments

@kinelski
Copy link
Member

Consider updating implementations where applicable for Key Vault LROs (follow-up of #19105).

Useful links:

@kinelski kinelski added KeyVault Client This issue points to a problem in the data-plane of the library. labels Jul 27, 2021
@kinelski kinelski added this to the Backlog milestone Jul 27, 2021
@heaths
Copy link
Member

heaths commented Jul 27, 2021

We only have 1 true LRO. The others are pseudo-LROs that poll for state and cannot use OperationInternal directly.

As for CertificateOperation, the service returns a non-standard RetryAfter header (no dash) so we'd need to be able to override that for any possible benefit. Could changes be made to OperationInternal to provide some customization, e.g. make the class partial and let us supply retry headers or something?

/cc @pakrym

@kinelski
Copy link
Member Author

Sure. We could make it an editable list of headers (maybe two lists depending on whether it counts seconds or milliseconds). Would it be worth adding the header directly to the OperationInternal class? Do you know of any other services using the same header?

@heaths
Copy link
Member

heaths commented Jul 29, 2021

I suppose it really depends on any potential perf impact of making the header list extensible vs. adding it. I would hope Key Vault is the only service sending back "RetryAfter". Even if we did add it to the list of supported headers, I would add it last to avoid impacting the perf of all the other services. In fact, I wonder if we can glean from telemetry what the right order should be to optimize for services that send one over another. Or, I suppose, use a HashSet but with only a few headers that, too, is probably more expensive than searching an O(N) list of a few items.

@heaths heaths modified the milestones: Backlog, [2021] December Dec 1, 2021
@heaths
Copy link
Member

heaths commented Dec 1, 2021

After talking with @pakrym about #20667 we could probably take this one for at least the pseudo-LROs. Pulling it in for consideration.

@heaths
Copy link
Member

heaths commented Dec 1, 2021

FWIW, Key Vault is sending back Retry-After and not RetryAfter, nor can I find any evidence it ever did send RetryAfter. 😕

heaths added a commit to heaths/azure-sdk-for-net that referenced this issue Dec 1, 2021
heaths added a commit to heaths/azure-sdk-for-net that referenced this issue Dec 1, 2021
heaths added a commit that referenced this issue Dec 2, 2021
* Use operation helpers

Partially fixes #22912

* Resolve PR feedback

* Add OperationInternal tests

* Use OperationInternal for all pseudo-LROs

Resolves #22912

* Use KeyVaultPipeline for SecretClient

Resolves #8115

* Deduplicate identifier parsing

Resolves #24262

* Resolve PR feedback
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants