Update Azure.Core shared for LRO rehydration #41088

live1206 · 2024-01-09T02:06:05Z

Construct RehydrationToken from multiple fields
- HeaderSource: Indicates where the NextRequestUri locates in the initial response header, it could be OperationLocation, AzureAsyncOperation, Location or None
- NextRequestUri: The Uri to poll status of the operation
- InitialUri: The initial Uri of the operation
- RequestMethod: Http request method of the operation
- LastKnowLocation: The last known Location value in the response header
- FinalStateVia: Configuration from API spec, it could be AzureAsyncOperation, Location, OriginalUri or OperationLocation
For fake delete LRO with 404, just return empty response with 204 because the delete succeeded
To support LRO rehydration for both DPG and MPG, make Operation non-abstract

azure-sdk · 2024-01-09T02:17:29Z

API change check

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

KrzysztofCwalina · 2024-01-24T19:05:04Z

Can we eliminate some of the shared source files? We consider shared sources to be engineering debt.

… type

annelo-msft · 2024-03-12T21:17:54Z

sdk/core/Azure.Core/api/Azure.Core.net461.cs

        public override string ToString() { throw null; }
    }
-    public abstract partial class Operation
+    public partial class Operation


What motivates making this type non-abstract? This is a big change to a central Core type, to it would be good to be crisp on the reasons that motivate this.

One suggestion is to add these APIs to an internal subtype of Operation and return that anywhere this instance is needed. I believe we would prefer to keep the Operation type itself internal, as that lets it evolve better in the future, helps us address performance issues, etc.

Sorry, did not add enough explanation for this.
The goal is to implement LRO rehydration for both DPG and MPG.
Had a discussion with @m-nash , @KrzysztofCwalina and @tg-msft, given the way we are implementing LRO for DPG nowadays, there is no instantiable type for DPG users, only Operation available.
That's why we want to make Operation non-abstract.

I think we have two choices: 1) we could make the type non-abstract as in this PR, 2) we could add a static method Operation.Rehydrate that returns an internal subtype of Operation. I know that we initially said that we wanted a ctor, but now that I think about it the static method could be better. Rehydration is an advanced scenario and the static method would hide the API a bit. Also, it would allow us to not have to add fields to Operation

OK. Static method is better to me as well. Given we can't add type constraint to constructor directly, we need to throw in constructor.
I will create another PR to implement it with static method to not mess with the existing change.

Created #42686 for static methods

annelo-msft · 2024-03-12T21:21:25Z

sdk/core/Azure.Core/api/Azure.Core.net461.cs

        protected Operation() { }
-        public abstract bool HasCompleted { get; }
-        public abstract string Id { get; }
+        public Operation(Azure.Core.Pipeline.HttpPipeline pipeline, Azure.Core.RehydrationToken? rehydrationToken, Azure.Core.ClientOptions options = null) { }


It would be preferable not to add a dependency of an end-user type (Operation) on a client-internal type (HttpPipeline). Is there any way we could avoid this? If you don't know offhand, we might be able to help you find design-alternatives if you could share the scenario that motivates the addition of this API.

The actual implementation is hold in NextLinkOperationImplementation, so make Operation non-abstract, we need to implement the LRO rehydration based on NextLinkOperationImplementation.
To be able to construct NextLinkOperationImplementation, we need the user to input HttpPipeline and RehydrationToken, as construction here.

annelo-msft · 2024-03-12T21:22:18Z

sdk/core/Azure.Core/api/Azure.Core.net461.cs

        protected Operation() { }
-        public abstract bool HasValue { get; }
-        public abstract T Value { get; }
+        public Operation(Azure.Core.Pipeline.HttpPipeline pipeline, Azure.Core.RehydrationToken? rehydrationToken, Azure.Core.ClientOptions options = null) { }


Same comment that we should work to avoid adding a dependency on HttpPipeline in this type's public API.

sdk/core/Azure.Core/src/RehydrationToken.Serialization.cs

annelo-msft · 2024-03-12T21:26:47Z

sdk/core/Azure.Core/src/Shared/NextLinkOperationImplementation.cs

+
+            if (!lroDetails.TryGetValue("nextRequestUri", out var nextRequestUri))
+            {
+                throw new InvalidOperationException("Invalid next request URI from rehydration token");


Is this the correct exception type?

Update to ArgumentException

annelo-msft · 2024-03-12T21:29:08Z

sdk/core/Azure.Core/src/Shared/OperationInternal.cs

        public OperationInternal(IOperation operation,
            ClientDiagnostics clientDiagnostics,
-            Response rawResponse,
+            Response? rawResponse,


What scenario motivates passing a null raw response? It looks like Operation.GetRawResponse has a non-nullable return type. Are you planning to return null from this method? I believe that would violate the contract with the end user.

This is the case of LRO rehydration used here, the user doesn't have response in place since the operation is not complete.
And if we take a look at the documentation of rawResponse here, it says "When a user instantiates an directly using a public constructor, there's no previous service call. In this case, passing null is expected."
So, I think it should be nullable in the first place.

Created #42666 for this change.

annelo-msft · 2024-03-12T21:31:29Z

sdk/core/Azure.Core/src/Shared/OperationInternalOfT.cs

        }

-        public override Response RawResponse => _stateLock.TryGetValue(out var state) ? state.RawResponse : _rawResponse;
+        public override Response RawResponse => (_stateLock.TryGetValue(out var state) ? state.RawResponse : _rawResponse) ?? throw new InvalidOperationException("The operation does not have a response yet. Please call UpdateStatus or WaitForCompletion first.");


Who calls this API? If it's internal, would it make sense to provide a tester rather than throwing an exception? It is not idiomatic to .NET to throw exceptions from properties - we should avoid doing this if possible.

The case is, the operation is not complete yet, there is no response for the user to consume.
The user need to update the status till complete firstly.

You are right, we should make this nullable instead and check if null in the usage.

Actually, we throw on incomplete operation for OperationInternal.Value. To be consistent, we should throw on this as well.

Get latest state during rehydration to avoid empty raw response, implemented in #42686

annelo-msft · 2024-03-12T21:33:17Z

sdk/core/Azure.Core/src/Shared/OperationInternalOfT.cs

            throw state.OperationFailedException!;
        }

+        private class EmptyResponse : Response


Ideally, we would avoid adding new internal subtypes of Response. Is there one in the Azure.Core code base you can reuse? If not, it would be good to add a comment to the type to explain why it exists and why it is unique, when you would expect to return it to an end-user, and what they would expect from it, so that maintainers of this code no what they can and can't change going forward.

This is only used for fake delete LRO, we just want to return an empty response with 204 to the user for this case.

Added the above comment to the class documentation. I can't find any existing internal implemented Response type for this usage.

annelo-msft · 2024-03-12T21:41:38Z

sdk/core/Azure.Core/tests/OperationTests.cs

+            Assert.Null(operation.GetRehydrationToken());
+        }
+
+        [Test]


If we move forward with making the Operation type non-abstract, this test file should contain full coverage of all implementation details of the implementation.

Tried to add tests for some properties of Operation and OperationOfT.
We need to mock HttpPipeline methods to be able to test UpdateStatus.

This should be possible with the TestFramework in Azure.Core - let us know if you need any help with this.

Thanks, I will dig around firstly, and get back if need any help.

Implemented in #42686

live1206 · 2024-03-14T07:33:49Z

Based on #41088 (comment), we would like to expose the LRO rehydration functionality as static method, created #42686 for it.

live1206 · 2024-03-29T02:40:31Z

continue the work in #42686

Update Azure.Core for LRO rehydration

7613a82

live1206 added the Mgmt This issue is related to a management package. label Jan 9, 2024

live1206 added 2 commits January 9, 2024 10:35

update

108ada7

update HLC

9518c51

live1206 force-pushed the lro-rehydration-core branch from d06f4ab to 9518c51 Compare January 9, 2024 09:51

live1206 added 12 commits January 9, 2024 20:37

fix test

bfd7490

Merge branch 'main' into lro-rehydration-core

6a5e480

Merge branch 'main' into lro-rehydration-core

6f484c5

Add GetRehydrationToken method

f4cdc1e

update

2b9e1aa

fix

74631b9

fix

a505ca0

fix

125ee0e

update API

b4bb5e0

fix

9bcc44e

fix

f69fe9a

fix

09eca6b

live1206 added 11 commits January 26, 2024 13:30

WIP

82448ef

Add GetRehydrationToken to Operation for rehydration purpose

cc68617

export API

b425271

make struct readonly

74183b1

export API

1530b50

update

ddc3f57

Merge branch 'rehydration-token' into lro-rehydration-core

5581f11

merge

d04a39b

update

f15eb00

move EmptyResponse from internal share to private

d715ab9

clean up

4e516e0

live1206 added 5 commits March 12, 2024 15:10

Fix AOT warning

3e71f0a

Make RehydrationToken nullable in constructor to comply with property…

a56b957

… type

update API

02b735c

update

90d3485

Merge branch 'feature/lro-rehydration-core' into lro-rehydration-core

e4379c5

annelo-msft reviewed Mar 12, 2024

View reviewed changes

sdk/core/Azure.Core/src/RehydrationToken.Serialization.cs Outdated Show resolved Hide resolved

annelo-msft reviewed Mar 12, 2024

View reviewed changes

live1206 and others added 2 commits March 13, 2024 11:41

update

dc90663

Make RawResponses nullable in OperatoinInternal

4e88474

live1206 mentioned this pull request Mar 13, 2024

Make rawResponse input nullable in OperatoinInternalOfT #42666

Closed

live1206 and others added 8 commits March 13, 2024 13:59

Merge branch 'response-nullable' into lro-rehydration-core

a8b7c0a

Add implementation for OperationInternalOfT

cdf4310

throw instead of nullable for property

6bc5760

Merge branch 'response-nullable' into lro-rehydration-core

bd09a8b

update test

0e1f069

fix test

dd2ee89

fix AOT

2195045

fix AOT

ac6d6ee

live1206 marked this pull request as draft March 14, 2024 08:54

live1206 closed this Mar 29, 2024

live1206 deleted the lro-rehydration-core branch September 2, 2024 05:19

Uh oh!

Update Azure.Core shared for LRO rehydration #41088

Update Azure.Core shared for LRO rehydration #41088

Uh oh!

Conversation

live1206 commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azure-sdk commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrzysztofCwalina commented Jan 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

live1206 Mar 13, 2024 • 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!

live1206 Mar 14, 2024 • 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!

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!

live1206 Mar 13, 2024 • 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!

live1206 Mar 13, 2024 • 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!

live1206 Mar 13, 2024 • 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!

live1206 commented Mar 14, 2024

Uh oh!

live1206 commented Mar 29, 2024

Uh oh!

live1206 commented Jan 9, 2024 •

edited

Loading

azure-sdk commented Jan 9, 2024 •

edited

Loading

live1206 Mar 13, 2024 •

edited

Loading

live1206 Mar 14, 2024 •

edited

Loading

live1206 Mar 13, 2024 •

edited

Loading

live1206 Mar 13, 2024 •

edited

Loading

live1206 Mar 13, 2024 •

edited

Loading