-
Notifications
You must be signed in to change notification settings - Fork 5
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
Make settings more usable #57
Conversation
…s guards for all setters
TimeSpan overallTimeout, | ||
TimeSpan timeoutPerTry, | ||
RetryPolicySettings retryPolicyPolicySettings, | ||
CircuitBreakerPolicySettings circuitBreakerPolicyPolicySettings) |
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 concern about the usability of this type.
Look:
I'm a client and create a new instance of CircuitBreakerPolicySettings
to use this constructor. It looks strange that I cannot set up OnBreak/OnReset/OnHalfOpen
in this instance directly, can it? What do you think?
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 thought about it. Here we have a trade off between usability and "correctness" I could say. If we add OnBreak/OnReset/OnHalfOpen
back to the CircuiteBreakerPolicySettings
, we have to choose:
- Remove it from the root settings as it was before.
- Deal with inconsistent state. I mean settings creation like this:
var cbSettings = new CircuitBreakerPolicySettings
{
OnBreak = myOnBreakHandler1,
};
var settings = new ResiliencePolicySettings
{
OnBreak = myOnBreakHandler2,
CircuitBreakerPolicySettings = cbSettings,
}
How should we choose between this two handlers? Last wins - could be confusing, because CB settings could be created somewhere in other place. Decorate CB OnBreak with root settings OnBreak? Possible, but overcomplicated in my opinion.
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 have one more idea about it. One of the major differences between our library and Polly is that Polly is like a Lego and your can build your own policies step-by-step choosing proper building blocks, but we provide a monolithic solution. You can tune it a bit, but you can't change the flow. In case of Polly it makes sense to keep OnRetry
action inside the RetryPolicy
because you can use it alone without other things, but in our case you can't choose to use or not one of the inner policies. It means, that our OnRetry
action is actually top level action. Your choice is not to subscribe or not to action provided by RetryPolicy, you choose subscribe or not to Dodo.HttpClient.ResiliencePolicy OnRetry action. It is important difference.
@@ -2,16 +2,16 @@ | |||
|
|||
namespace Dodo.HttpClientResiliencePolicies.TimeoutPolicy | |||
{ | |||
public sealed class OverallTimeoutPolicySettings : ITimeoutPolicySettings | |||
public class TimeoutPolicySettings |
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 think we should leave this class sealed
to avoid misunderstanding the concept.
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.
Also, the second thought, maybe this is an excess class because we don't have it in any public constructor at ResiliencePolicySettings. How I as a client can use it?
Two way here:
- Delete it (I know this was my idea to satisfy my wants for uniformity but it crashes by reality)
- Make it internal 😕
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.
Yes, you are right it should be sealed
. I will fix that.
I thought about this class too. Class itself looks nice because we keep all settings in the similar manner, but when I am working on tests refactoring, creating new instance each time and passing there single argument looks boring. I think we could keep it internal and use inside the library, but the end-user should provide only TimeSpan
structures. I will fix that.
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 have made changes to this thing. I am playing a bit with TimeoutPolicy and decide to remove it. Code looks cleaner now. Also I leave only default constructor in the ResiliencePoliciesSettings
because the overriden constructor solves the only problem of matching TimeSpan on TimeoutPolicySettings.
[+] Move
OnRetry
,OnBreak
,OnReset
andOnHalfOpen
actions to theResiliencePoliciesSettings
class. It allows us to initialize settings like this:[-]: Make all this actions internal in
RetryPolicySettings
andCircuitBreakerPolicySettings
to avoid collisions when user possibly may set for exampleOnRetry
action twice in different places.[+]: Add guards to policy setters in
ResiliencePoliciesSettings
to avoid situation when the user passnull
value to some settings. [+]: Policy setters also handle situations when for exampleOnRetry
action property set first and thenRetryPolicySettings
set with new value.[+]: Add some tests for this logic.
[-]: Get rid of
IRetryPolicySettings
,ICircuitBreakerPolicySettings
andITimeoutPolicySettings
interfaces, because they allows users to create their own implementations which should work incorrectly.Closes #52