search

Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.17k stars 4.53k forks source link

Consider not throwing for completed but unsuccessful long-running operations #21312

Closed pakrym closed 6 months ago

pakrym commented 3 years ago

https://github.com/Azure/autorest.csharp/issues/464

From @heaths :

FWIW, Krzysztof asked that we not throw during UpdateStatus for supported status codes (e.g. 200, 202, 404) and similar calls - only when getting a value (similar to Nullable). I believe Pavel was part of this discussion as well. As a result - with oversight from Krzysztof, the Key Vault pseudo-LROs are what Krzysztof wanted: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/keyvault/Azure.Security.KeyVault.Keys/src/DeleteKeyOperation.cs

pakrym commented 3 years ago

@kinelski pointed out that this would be against our current guidelines.

cc @christothes who worked on https://github.com/Azure/azure-sdk/pull/2003

heaths commented 3 years ago

When I implemented the LROs for Key Vault (real and pseudo-LROs) back in 2019, @KrzysztofCwalina recommended we throw from Value much like Nullable<T>.Value. If recommendations have changed, please ignore my comment.

pakrym commented 3 years ago

No, I think you were right. The Value should throw but I don't think the UpdateStatus should.

annelo-msft commented 11 months ago

@tg-msft: it would be good to scope this to DPG libraries; consider pointing customers to RequestContext to control error experience.

It would be good to understand the experience in the HLC libraries as well.

pallavit commented 6 months ago

Do we see us doing this for exsiting HLCs? For DPG and newer light-up APIs we already have a means to achieve this behavior albeit as an advanced scenario.

Do we see us changing this to mainstream scenario however? /cc: @annelo-msft

annelo-msft commented 6 months ago

@pallavit: @JoshLove-msft is driving this investigation and can make a recommendation based on his findings once available.

pallavit commented 6 months ago

@JoshLove-msft could you please share your current thinking here?

JoshLove-msft commented 6 months ago

The scope of my investigation was to determine whether or not the current shared source implementation actually aligns with the current guidelines and whether the guidelines may need updating to reflect the behavior.

The guidelines say that UpdateStatus should throw if the operation completed with a failed result. They also state that accessing the Operation Value property should rethrow the same exception for failed operations:

If the Value property is evaluated after the operation failed (HasValue is false and HasCompleted is true) throw the same exception as the one thrown when the operation failed.

The shared source implementation throws on failed results in UpdateStatus. It also throws the same exception when accessing Value.

So our official guidance in the implementation guidelines matches up with the Azure.Core shared source implementation.

The problem is that many LRO's created before the Azure.Core shared source implementation, such as Key Vault's , did the implementation themselves and do not follow the guidelines of throwing on a failed result from UpdateStatus. Changing this behavior from not throwing to throwing is a breaking change. I would recommend that these be left as is.

I do think there is room for improvement by adding a static analyzer. A simple analyzer could verify these conditions for any Operation subtype. For existing LROs that don't follow the pattern the analyzer can be disabled on a case by case basis. We could either repurpose this issue to track the static analyzer work, or open a separate issue for that and close this issue. @pallavit, any preference there?

pallavit commented 6 months ago

Thank you for sharing the details @JoshLove-msft

I do think there is room for improvement by adding a static analyzer. A simple analyzer could verify these conditions for any Operation subtype. For existing LROs that don't follow the pattern the analyzer can be disabled on a case by case basis. We could either repurpose this issue to track the static analyzer work, or open a separate issue for that and close this issue. @pallavit, any preference there?

As for this, I think we should open a bug in azure-sdk-tools for static analyzer. I agree with you that older SDKs would likely not be updated due to breaking changes concerns however this may be valuable for net new SDKs branded as well as other 3rd party scenarios in future. Even though LROs is a azure specific concept it may be worth evaluating it for the new toolchain as well so having a static analyzer bug for this will be useful. I would suggest tagging that issue with System.ClientModel as well so we think this scenario from that perspective as well. Let me know what you think?

JoshLove-msft commented 6 months ago

Works for me - filed https://github.com/Azure/azure-sdk-tools/issues/7478.

annelo-msft commented 6 months ago

[like] Anne Loomis Thompson reacted to your message:

From: JoshLove-msft @.> Sent: Wednesday, December 20, 2023 6:43:05 PM To: Azure/azure-sdk-for-net @.> Cc: Subscribed @.>; Comment @.> Subject: Re: [Azure/azure-sdk-for-net] Consider not throwing for completed but unsuccessful long-running operations (Issue #21312)

Works for me - filed #21312 (comment)https://github.com/Azure/azure-sdk-for-net/issues/21312#issuecomment-1864920346.

— Reply to this email directly, view it on GitHubhttps://github.com/Azure/azure-sdk-for-net/issues/21312#issuecomment-1864965329 or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFCIWYZGNQXJCWOI33E4X6DYKMWTXBFKMF2HI4TJMJ2XIZLTSWBKK5TBNR2WLJDUOJ2WLJDOMFWWLO3UNBZGKYLEL5YGC4TUNFRWS4DBNZ2F6YLDORUXM2LUPGBKK5TBNR2WLJDUOJ2WLJDOMFWWLLTXMF2GG2C7MFRXI2LWNF2HTAVFOZQWY5LFUVUXG43VMWSG4YLNMWVXI2DSMVQWIX3UPFYGLAVFOZQWY5LFVIYTCNZYGQ2DOOBWGCSG4YLNMWUWQYLTL5WGCYTFNSBKK5TBNR2WLKRRGIZDKOBYG44DIOFENZQW2ZNJNBQXGX3MMFRGK3FMON2WE2TFMN2F65DZOBS2YSLTON2WKQ3PNVWWK3TUUZ2G64DJMNZZJAVEOR4XAZNKOJSXA33TNF2G64TZUV3GC3DVMWTTEOJSHA4TINECUR2HS4DFUVUXG43VMWSXMYLMOVS2SOJQGAYTCMRYGU2IFJDUPFYGLJLMMFRGK3FFOZQWY5LFVIYTCNZYGQ2DOOBWGCBKI5DZOBS2K3DBMJSWZJLWMFWHKZNKGEZDENJYHA3TQNBYU52HE2LHM5SXFJTDOJSWC5DF. You are receiving this email because you commented on the thread.

Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.