-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: add basic retry middleware #283
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
using System; | ||
using Grpc.Core; | ||
using Microsoft.Extensions.Logging; | ||
|
||
namespace Momento.Sdk.Config.Retry | ||
{ | ||
public interface IRetryEligibilityStrategy | ||
{ | ||
public ILoggerFactory? LoggerFactory { get; } | ||
public IRetryEligibilityStrategy WithLoggerFactory(ILoggerFactory loggerFactory); | ||
public bool IsEligibleForRetry<TRequest>(Status status, TRequest request) where TRequest : class; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
using System; | ||
using System.Threading.Tasks; | ||
using Grpc.Core; | ||
using Microsoft.Extensions.Logging; | ||
using Momento.Sdk.Config.Middleware; | ||
using System.Linq; | ||
using Momento.Protos.CacheClient; | ||
using System.Collections.Generic; | ||
|
||
namespace Momento.Sdk.Config.Retry | ||
{ | ||
internal class RetryMiddleware : IMiddleware | ||
{ | ||
public ILoggerFactory? LoggerFactory { get; } | ||
|
||
private readonly ILogger _logger; | ||
private readonly IRetryStrategy _retryStrategy; | ||
|
||
public RetryMiddleware(ILoggerFactory loggerFactory, IRetryStrategy retryStrategy) | ||
{ | ||
LoggerFactory = loggerFactory; | ||
_logger = loggerFactory.CreateLogger<RetryMiddleware>(); | ||
_retryStrategy = retryStrategy; | ||
} | ||
|
||
public RetryMiddleware WithLoggerFactory(ILoggerFactory loggerFactory) | ||
{ | ||
return new(loggerFactory, _retryStrategy); | ||
} | ||
|
||
IMiddleware IMiddleware.WithLoggerFactory(ILoggerFactory loggerFactory) | ||
{ | ||
return WithLoggerFactory(loggerFactory); | ||
} | ||
|
||
public async Task<MiddlewareResponseState<TResponse>> WrapRequest<TRequest, TResponse>( | ||
TRequest request, | ||
CallOptions callOptions, | ||
Func<TRequest, CallOptions, Task<MiddlewareResponseState<TResponse>>> continuation | ||
) where TRequest : class where TResponse : class | ||
{ | ||
var foo = request.GetType(); | ||
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. this should be removed |
||
MiddlewareResponseState<TResponse> nextState; | ||
int attemptNumber = 0; | ||
int? retryAfterMillis = 0; | ||
do | ||
{ | ||
var delay = retryAfterMillis ?? 0; | ||
if (delay > 0) | ||
{ | ||
await Task.Delay(delay); | ||
} | ||
attemptNumber++; | ||
nextState = await continuation(request, callOptions); | ||
Comment on lines
+53
to
+54
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. I'm not sure I see how this will disambiguate between the likes of ListPushBack() or DictionaryIncrement() (non-idempotent methods) and Get() or Set() (idempotent methods). What's your plan there? 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. See the code in 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. I completely missed |
||
|
||
// NOTE: we need a try/catch block here, because: (a) we cannot call | ||
// `nextState.GetStatus()` until after we `await` the response, or | ||
// it will throw an error. and (b) if the status is anything other | ||
// than "ok", the `await` on the response will throw an exception. | ||
try | ||
{ | ||
await nextState.ResponseAsync; | ||
|
||
if (attemptNumber > 1) | ||
{ | ||
_logger.LogDebug($"Retry succeeded (attempt {attemptNumber})"); | ||
} | ||
break; | ||
} | ||
catch (Exception) | ||
{ | ||
var status = nextState.GetStatus(); | ||
_logger.LogDebug($"Request failed with status {status.StatusCode}, checking to see if we should retry; attempt Number: {attemptNumber}"); | ||
_logger.LogTrace($"Failed request status: {status}"); | ||
retryAfterMillis = _retryStrategy.DetermineWhenToRetryRequest(nextState.GetStatus(), request, attemptNumber); | ||
} | ||
} | ||
while (retryAfterMillis != null); | ||
|
||
return new MiddlewareResponseState<TResponse>( | ||
ResponseAsync: nextState.ResponseAsync, | ||
ResponseHeadersAsync: nextState.ResponseHeadersAsync, | ||
GetStatus: nextState.GetStatus, | ||
GetTrailers: nextState.GetTrailers | ||
); | ||
} | ||
} | ||
} | ||
|
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.
Here and elsewhere I notice you're charging the users' runtimes string interpolation allocations unconditionally. I think you should rewrite all these LogDebugs like:
see for more examples and detail: https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.logging.loggerextensions.logdebug?view=dotnet-plat-ext-6.0
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.
good call, agreed, it was my intent to write them that way but I always forget. thx
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.
kotlin-structured-logging is an addictive library. It's so nice to not have to care about anything w.r.t. logging invocation cost inside of those logging blocks. I catch myself writing logs like this too so I look out for them
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.
i hope we will have a rust-structured-logging (or maybe one already exists in the rust universe)