-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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] Improve OperationHelpers used by our LROs #19105
Changes from 25 commits
57ae23f
5b901ce
40d6450
e7046fd
e8cc4d4
adfb122
a5f4284
7727103
2664ce8
90b8f44
d2efbb2
cf75dd6
43bc320
d88ea8a
1ebb67f
d3b2e32
88a8b55
e8b467e
bcd9e65
f9681d8
ce7f2bf
177728e
1c6c8ad
c367892
7265d61
91744d5
30bed6a
3ec6997
10e2d11
c24a61f
d693567
cd68031
c13cc96
db816ea
0bba0d0
caed979
3ee4d45
2bf26c7
7fb7bce
41de045
02fe9d8
9b55b43
6943a0d
4fdaeb6
bebbbbd
eb2401e
63262b7
4a4fc76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,215 @@ | ||||||||||||||||||||||||||||||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||||||||||||||||||||||||||||||
// Licensed under the MIT License. | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
using System; | ||||||||||||||||||||||||||||||
using System.Collections.Generic; | ||||||||||||||||||||||||||||||
using System.Threading; | ||||||||||||||||||||||||||||||
using System.Threading.Tasks; | ||||||||||||||||||||||||||||||
using Azure.Core.Pipeline; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
namespace Azure.Core | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
internal class OperationInternal<TResult> | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
private const string RetryAfterHeaderName = "Retry-After"; | ||||||||||||||||||||||||||||||
kinelski marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
private const string RetryAfterMsHeaderName = "retry-after-ms"; | ||||||||||||||||||||||||||||||
private const string XRetryAfterMsHeaderName = "x-ms-retry-after-ms"; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
private readonly IOperation<TResult> _operation; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
private readonly ClientDiagnostics _diagnostics; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
private TResult _value; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
private RequestFailedException _operationFailedException; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public OperationInternal(ClientDiagnostics clientDiagnostics, IOperation<TResult> operation) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
_operation = operation; | ||||||||||||||||||||||||||||||
_diagnostics = clientDiagnostics; | ||||||||||||||||||||||||||||||
OperationTypeName = operation.GetType().Name; | ||||||||||||||||||||||||||||||
DefaultPollingInterval = TimeSpan.FromSeconds(1); | ||||||||||||||||||||||||||||||
pakrym marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
ScopeAttributes = new Dictionary<string, string>(); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public IDictionary<string, string> ScopeAttributes { get; } | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public bool HasValue { get; private set; } | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public bool HasCompleted { get; set; } | ||||||||||||||||||||||||||||||
kinelski marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public TResult Value | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
get | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
if (HasValue) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
return _value; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
else if (_operationFailedException != null) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
throw _operationFailedException; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
throw new InvalidOperationException("The operation has not completed yet."); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
set | ||||||||||||||||||||||||||||||
kinelski marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
if (value is null) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
throw new ArgumentNullException(nameof(Value)); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
_value = value; | ||||||||||||||||||||||||||||||
HasValue = true; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public Response RawResponse { get; set; } | ||||||||||||||||||||||||||||||
kinelski marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public string OperationTypeName { get; set; } | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public TimeSpan DefaultPollingInterval { get; set; } | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public async ValueTask<Response> UpdateStatusAsync(CancellationToken cancellationToken) => | ||||||||||||||||||||||||||||||
await UpdateStatusAsync(async: true, cancellationToken).ConfigureAwait(false); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public Response UpdateStatus(CancellationToken cancellationToken) => | ||||||||||||||||||||||||||||||
UpdateStatusAsync(async: false, cancellationToken).EnsureCompleted(); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public async ValueTask<Response<TResult>> WaitForCompletionAsync(CancellationToken cancellationToken) => | ||||||||||||||||||||||||||||||
await WaitForCompletionAsync(DefaultPollingInterval, cancellationToken).ConfigureAwait(false); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public async ValueTask<Response<TResult>> WaitForCompletionAsync(TimeSpan pollingInterval, CancellationToken cancellationToken) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
while (true) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
Response response = await UpdateStatusAsync(cancellationToken).ConfigureAwait(false); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
if (HasCompleted) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
return Response.FromValue(Value, response); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
var serverDelay = GetServerDelay(response); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
var delay = serverDelay > pollingInterval | ||||||||||||||||||||||||||||||
kinelski marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
? serverDelay : pollingInterval; | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, it seems there's no overload for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there an optimization reason to prefer |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
await WaitAsync(delay, cancellationToken).ConfigureAwait(false); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
protected virtual async Task WaitAsync(TimeSpan delay, CancellationToken cancellationToken) => | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this virtual and nothing else is? If anything, other methods like UpdateStatus/Async make more sense as virtual methods than simply waiting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're making it virtual for testing only. The current design does not expect developers to derive from |
||||||||||||||||||||||||||||||
await Task.Delay(delay, cancellationToken).ConfigureAwait(false); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
private async ValueTask<Response> UpdateStatusAsync(bool async, CancellationToken cancellationToken) | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flow of
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add as a comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, I'd like to see more comments on internal types/members anyway - especially those in shared source like in Core/Shared. What exceptions might we expect? What parameters are actually required? What is the behavior, etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added (a lot of) docstrings to the methods we expect developers to use. We still intend to add a usage guide in our docs in another PR. |
||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
using DiagnosticScope scope = _diagnostics.CreateScope($"{OperationTypeName}.UpdateStatus"); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
foreach (KeyValuePair<string, string> attribute in ScopeAttributes) | ||||||||||||||||||||||||||||||
kinelski marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
scope.AddAttribute(attribute.Key, attribute.Value); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
scope.Start(); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
try | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
return await UpdateStateAsync(async, cancellationToken).ConfigureAwait(false); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
catch (RequestFailedException e) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
scope.Failed(e); | ||||||||||||||||||||||||||||||
throw; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
catch (Exception e) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
var requestFailedException = new RequestFailedException("Unexpected failure.", e); | ||||||||||||||||||||||||||||||
kinelski marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
scope.Failed(requestFailedException); | ||||||||||||||||||||||||||||||
throw requestFailedException; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
private async ValueTask<Response> UpdateStateAsync(bool async, CancellationToken cancellationToken) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
var state = await _operation.UpdateStateAsync(async, cancellationToken).ConfigureAwait(false); | ||||||||||||||||||||||||||||||
kinelski marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
RawResponse = state.RawResponse; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
if (state.Succeeded == true) | ||||||||||||||||||||||||||||||
kinelski marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
Value = state.Value; | ||||||||||||||||||||||||||||||
HasCompleted = true; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
else if (state.Succeeded == false) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
_operationFailedException = state.OperationFailedException ?? (async | ||||||||||||||||||||||||||||||
? await _diagnostics.CreateRequestFailedExceptionAsync(state.RawResponse).ConfigureAwait(false) | ||||||||||||||||||||||||||||||
: _diagnostics.CreateRequestFailedException(state.RawResponse)); | ||||||||||||||||||||||||||||||
HasCompleted = true; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
throw _operationFailedException; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
return state.RawResponse; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
private static TimeSpan GetServerDelay(Response response) | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth sharing this implementation with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Edit: took another look and |
||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
if (response.Headers.TryGetValue(RetryAfterMsHeaderName, out string retryAfterValue) | ||||||||||||||||||||||||||||||
|| response.Headers.TryGetValue(XRetryAfterMsHeaderName, out retryAfterValue)) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
if (int.TryParse(retryAfterValue, out int serverDelayInMilliseconds)) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
return TimeSpan.FromMilliseconds(serverDelayInMilliseconds); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
if (response.Headers.TryGetValue(RetryAfterHeaderName, out retryAfterValue)) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
if (int.TryParse(retryAfterValue, out int serverDelayInSeconds)) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
return TimeSpan.FromSeconds(serverDelayInSeconds); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
return TimeSpan.Zero; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
internal interface IOperation<TResult> | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
ValueTask<OperationState<TResult>> UpdateStateAsync(bool async, CancellationToken cancellationToken); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
internal readonly struct OperationState<TResult> | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
private OperationState(Response rawResponse, bool? succeeded, TResult value, RequestFailedException operationFailedException) | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
RawResponse = rawResponse; | ||||||||||||||||||||||||||||||
Succeeded = succeeded; | ||||||||||||||||||||||||||||||
Value = value; | ||||||||||||||||||||||||||||||
OperationFailedException = operationFailedException; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public Response RawResponse { get; } | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public bool? Succeeded { get; } | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seeing this now (given my previous comment), if you don't actually care about the null state, you could handle that here in the helper e.g. public bool Succeeded => _succeeded.HasValue && _succeeded.Value; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to differentiate because we have three possible states:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, I get it. But I only remember seeing this property compared to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If this is what we are aiming for, I second Chris here that this should be documented, commented, etc so people don't have to figure out what each value means There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
azure-sdk-for-net/sdk/core/Azure.Core/src/Shared/OperationInternal.cs Lines 139 to 152 in db816ea
If it's true, we change the state. If it's false, we change it to something else. If it's null, we keep the state we had previously. We can't achieve the same behavior if we have a non-nullable bool given that we have three possible outcomes. I'll make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We'll have proper documentation and a migration guide, but developers won't need to understand these values directly. An
Its properties, such as |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public TResult Value { get; } | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public RequestFailedException OperationFailedException { get; } | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public static OperationState<TResult> Success(Response rawResponse, TResult value) => | ||||||||||||||||||||||||||||||
new OperationState<TResult>(rawResponse, true, value, default); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public static OperationState<TResult> Failure(Response rawResponse, RequestFailedException operationFailedException = null) => | ||||||||||||||||||||||||||||||
new OperationState<TResult>(rawResponse, false, default, operationFailedException); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public static OperationState<TResult> Pending(Response rawResponse) => | ||||||||||||||||||||||||||||||
new OperationState<TResult>(rawResponse, default, default, default); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming it
OperationInternal
for now. We can't useOperationHelpers
because we still have some clients using the name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently have:
OperationInternal<TResult>
andOperationInternal<TResult, TResponseType>
.OperationInternal<TResult>
receives aResponse
from the service, and exposes theValue
property as aTResult
.OperationInternal<TResult, TResponseType>
receives aResponse<ResponseType>
from the service, and exposes theValue
property as aTResult
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a plan to move everyone so eventually
OperationHelpers
class can be deleted and we avoid confusion?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. That's the plan. We'll probably keep the
OperationHelpers
name as well.