SE.Redis Reconnect retry policy #510
Conversation
|
@deepakverma that would be awesome - because right now all my Redis code is wrapped in retry logic |
NickCraver
left a comment
NickCraver
left a comment
There was a problem hiding this comment.
This looks good. Great improvement. It needs a just a few tweaks and comments to be good to go.
|
|
||
| /// <summary> | ||
| /// The retry policy to be used for connection reconnects | ||
| /// </summary> |
There was a problem hiding this comment.
Since this creates an object for the null case, the format should change a bit. Right now, it'll spin up a new LinearRetry on each property fetch - should capture it to the private when null, e.g. in the get:
return reconnectRetryPolicy ?? (reconnectRetryPolicy = new LinearRetry(ConnectTimeout));Sidenote: I don't recommend the static default option, since it'd be a bad long-term assumption of a stateless retry policy. Would just approach this with the tweak above.
| configCheckSeconds = configCheckSeconds, | ||
| responseTimeout = responseTimeout, | ||
| defaultDatabase = defaultDatabase, | ||
| ReconnectRetryPolicy = ReconnectRetryPolicy, |
There was a problem hiding this comment.
reconnectRetryPolicy = ReconnectRetryPolicy,(casing is off, it's hitting the prop)
| namespace StackExchange.Redis | ||
| { | ||
|
|
||
| public class ExponentialRetry : IReconnectRetryPolicy |
There was a problem hiding this comment.
Needs XML comments, telling people how this works, since it'll appear directly in their code.
| private int deltaBackOffMilliseconds; | ||
| private int maxDeltaBackOffMilliseconds = (int)TimeSpan.FromSeconds(10).TotalMilliseconds; | ||
| [ThreadStatic] | ||
| static private Random r; |
There was a problem hiding this comment.
private static (keyword order)
| { | ||
| } | ||
|
|
||
| public ExponentialRetry(int deltaBackOffMilliseconds, int maxDeltaBackOffMilliseconds) |
There was a problem hiding this comment.
XMl comments on all of these as well, so users know how it behaves.
| /// </summary> | ||
| /// <param name="currentRetryCount">The number of times reconnect retries have already been made by the multiplexer while it was in connecting state</param> | ||
| /// <param name="timeElapsedMillisecondsSinceLastRetry">Total time elapsed in milliseconds since the last reconnect retry was made</param> | ||
| bool ShouldRetry(long currentRetryCount, int timeElapsedMillisecondsSinceLastRetry); |
There was a problem hiding this comment.
I like the long here, giving this connection lots of chances :)
|
Thanks for the review, I have updated the pull request. |
|
@NickCraver what's the plan to merge this for folks who want to try it out? |
|
@deepakverma I'm merging in some Opserver bits. I'll try and pull this to test and merge tonight. I appreciate the PR and sorry we've been so busy lately :( |
|
Verified locally - thanks for the work! This will be in the next NuGet build. |
3 participants
Implemented reconnect retry policies to back off connectimeout wait time linearly (default) or exponentially
config.ReconnectRetryPolicy = new ExponentialRetry(config.ConnectTimeout);
Looking for code review and any other feedback on the pull request.
thx.