Issue 38 httpclientsettings refactoring #42
Conversation
src/Dodo.HttpClient.ResiliencePolicies/CircuitBreakerSettings/CircuitBreakerSettings.cs
Show resolved
Hide resolved
src/Dodo.HttpClient.ResiliencePolicies/RetrySettings/JitterRetrySettings.cs
Show resolved
Hide resolved
src/Dodo.HttpClient.ResiliencePolicies/RetrySettings/JitterRetrySettings.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public static IRetrySettings Default() => new JitterRetrySettings(Defaults.Retry.RetryCount); | ||
| public Action<DelegateResult<HttpResponseMessage>, TimeSpan> OnRetry { get; set; } | ||
|
|
||
| private static readonly TimeSpan _defaultMedianFirstRetryDelay = |
There was a problem hiding this comment.
Should we add MedianFirstRetryDelay as a public property too? It was possible to set it in the previous version of the JitterRetrySettings and it is important parameter for this type of settings.
There was a problem hiding this comment.
I think it doesn't make sense.
In the previous version, this parameter can be sent directly to sleepDurationProvider via constructor, and the property MedianFirstRetryDelay itself wasn't used.
int retryCount,
TimeSpan medianFirstRetryDelay,
Action<DelegateResult<HttpResponseMessage>, TimeSpan> onRetry)
{
RetryCount = retryCount;
SleepDurationProvider = _defaultSleepDurationProvider(retryCount, medianFirstRetryDelay);
OnRetry = onRetry;
}
The second thought is in this design version of settings classes If we add a public property MedianFirstRetryDelay we don't have control over when it will be filled and this means that we don't know when we should change the sleepDurationProvider.
| new JitterRetrySettings | ||
| { | ||
| RetryCount = retryCount, | ||
| SleepDurationProvider = JitterRetrySettings._defaultSleepDurationProvider(retryCount, TimeSpan.FromMilliseconds(50)) |
There was a problem hiding this comment.
It is not the same logic here as it was before. Actually we should fill absent MedianFirstRetryDelay and do not set SleepDurationProvider at all, because JutterRetrySettings will take correct provider by default. We should discuss it.
No description provided.