Skip to content
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

How do i access the policies within PolicyWrap? #312

Closed
udlose opened this issue Sep 14, 2017 · 8 comments
Closed

How do i access the policies within PolicyWrap? #312

udlose opened this issue Sep 14, 2017 · 8 comments

Comments

@udlose
Copy link

udlose commented Sep 14, 2017

If i pass a couple of policies to PolicyWrap, I'd like to be able to access them - possibly by name? For example, i have a RetryPolicy, CircuitBreakerPolicy, and FallbackPolicy passed to a PolicyWrap. I'm storing a reference to the PolicyWrap instead of the individual policies. I'd like to be able to access the CircuitBreakerPolicy to check its CircuitState and call Isolate() and Reset().

I'm not able to find anything exposing the inner policies. Am I missing something?

@rjongeneelen
Copy link
Contributor

rjongeneelen commented Sep 14, 2017

I don't believe PolicyWrap is exposing its inner policies, and I don't believe it should. In the end, it's just a new policy itself, which (recursively) wraps existing policy instances.

If I understand your use case correctly, the FallbackPolicy instance is the actual object you want to access. Personally, I guess it shouldn't matter if it's wrapped or not, or even if it's re-used at many different locations. So rather than trying to find the policy through a PolicyWrap, I would suggest keeping track of the FallbackPolicy itself, regardless of where it's used.

Perhaps you could use the PolicyRegistry and register the FallbackPolicy instance by name? Especially when using the PolicyRegistry in combination with DI, you could easily use the registry to find and access specific policy instances from anywhere within your application.

Just my 2 cents.

@udlose
Copy link
Author

udlose commented Sep 14, 2017

I need access to the CircuitBreaker within the PolicyWrap, not the Fallback. It's unfortunate that the inner policies aren't exposed as now I am forced to keep track of both the CircuitBreaker that's in the PolicyWrap and the PolicyWrap itself. I understand I can write my own implementation but I'd prefer to leverage something that's already an established pattern. BTW, I don't see anything wrong with adding some sort of GetPolicy(policyKey) method on PolicyWrap

How do others keep track of the CircuitState when using a CircuitBreakerPolicy within a PolicyWrap?

@rjongeneelen
Copy link
Contributor

Yes sorry, I meant to say CircuitBreaker policy, not Fallback policy.

@reisenberger
Copy link
Member

reisenberger commented Sep 15, 2017

Thank you @udlose and @ExtRemo75 for the great discussion on this!

Decoupling of policy configuration from usage due to DI, or using PolicyRegistry, does mean that code could be passed a PolicyWrap whose constituent parts it doesn't know, but wants to. It seems valid for PolicyWrap to expose this.

@udlose Re GetPolicy(string policyKey), I see your line of thinking there, but it does presuppose that the code receiving the PolicyWrap knows the key of the policy it wants to extract (so that still has to be transmitted/tracked separately).

How about an API which focuses simply on the essence of what PolicyWrap is:

(1) read-only property IsPolicy Outer returning the outer policy of the wrap (without recursive unpacking if that is a PolicyWrap)
(2) read-only property IsPolicy Inner returning the inner policy of the wrap (without recursive unpacking if that is a PolicyWrap)
(3) method IEnumerable<IsPolicy> GetPolicies() returning all policies of the wrap (correctly sequenced outer-to-inner, recursively unpacking and flattening if any is in turn a PolicyWrap)

From (3) anything like "extract the circuit-breaker" or "extract the Policy with this key" follows trivially with LINQ.

The current PolicyWrap stores only the implementation needed to execute on the PolicyWrap, so some private fields/read-only properties for outer and inner would need adding, probably only settable via the ctor.

What do you think?

PR to implement this would be welcome!

Cross-ref: https://stackoverflow.com/questions/45913775/ . (If we implement this, let's post also on that StackOverflow that it is done!)

@udlose
Copy link
Author

udlose commented Sep 20, 2017

@reisenberger Couple of thoughts:

  1. I really dislike the IsPolicy interface. It only exposes a key. To get anything of of a list of IsPolicy instances, you have to soft-cast them and check for null to find what you're looking for.
  2. What if you have more than one CB? Maybe this would not normally happen, but the API should account for it.
  3. It is certainly possible that I have more than one of the same type of policy in a PolicyWrap. In that case, how do I differentiate them other than PolicyKey?

@reisenberger
Copy link
Member

reisenberger commented Sep 24, 2017

@udlose Agreed, IsPolicy is not a very useful return type to work with.

IsPolicy is a thin interface because it is just a 'marker' interface for a policy which might be either a non-generic or a generic policy (each suits different scenarios; eliminating one or the other would cause pain points elsewhere). Since PolicyWrap allows combining non-generic and generic policies, IsPolicy is necessarily the lowest common denominator for what any element of a PolicyWrap might be.

However, completely agree that IsPolicy is not a very useful return type to actually work with. With PolicyRegistry we added helper methods to extract and cast policies to a more specific type, so how about we have similar for extracting from PolicyWrap?

So a revised overall API for this feature could be:

(1)-(2)-(3) as per my previous comment. These are important as they form the logical converse to constructing the PolicyWrap.

Plus:

(4) IEnumerable<TPolicy> GetPolicies<TPolicy>() // gets (and casts) all policies in the wrap, which are of type TPolicy
(5) IEnumerable<TPolicy> GetPolicies<TPolicy>(Func<TPolicy, bool>) // ditto with predicate
(6) TPolicy GetPolicy<TPolicy>() // gets the single policy in the wrap, of type TPolicy (for when you are confident you have only one)
(7) TPolicy GetPolicy<TPolicy>(Func<TPolicy, bool>) // ditto with predicate

And: [EDIT: For the reason you say, it would be useful to have this]

(8) TPolicy GetPolicy<TPolicy>(string policyKey)

Thoughts?

(Thanks for input so far, which is turning this into a better feature!)

@udlose
Copy link
Author

udlose commented Sep 27, 2017

@reisenberger I think most of those are good. of course any method taking a param would have to have null param validation on it which would go for (5), (7), and (8) (where this includes null check and string.IsNullOrWhitespace()). I think the methods returning IEnumerable<T> should return a non-null empty list if no such policies are found (.net Framework Design Guidelines). I'm not sure about the need for (6), (7) though. The caller could accomplish the same by doing a .Single(). If you do think it's more convenient to provide them then I'd suggest throwing an exception if more than one policy satisfying the criteria is found.

thoughts?

reisenberger added a commit to reisenberger/Polly that referenced this issue Nov 17, 2017
Implemtation for App-vNext#312 from @MartinSStewart , Polly Knowabunga Hackathon
at Tretton37
reisenberger added a commit that referenced this issue Nov 17, 2017
Implemtation for #312 from @MartinSStewart , Polly Knowabunga Hackathon
at Tretton37
@reisenberger reisenberger added this to the v5.6.0 milestone Nov 26, 2017
@reisenberger
Copy link
Member

This feature is delivered in Polly v5.6.0 (will be released via nuget in next day or so).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants