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

Adsk contrib/add circuit breaker policy interface #205

Conversation

christopherbahr
Copy link

We wanted to build something for our internal monitoring systems to track the current state of our active circuits but there isn't a nice way to hold a collection of CircuitBreakerPolicy<T> with different wrapped types. This adds an interface that allows for managing collections of policies.

We've done a workaround for the moment (just holding a collection of functions that return a CircuitState) but it would be a nice cleanup for us to have this.

@dnfclas
Copy link

dnfclas commented Jan 13, 2017

Hi @christopherbahr, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@reisenberger
Copy link
Member

Thank you v much for this @christopherbahr , completely see the problem it solves.

A question in my mind is how this interacts with the wider question of adding an IPolicy interface which exposes the Execute() methods. My instinct is that when users see an ICircuitBreakerPolicy interface (says: "this can be used as a circuit breaker policy"), they would expect it to allow them to call the various Execute() overloads (likely with ICircuitBreakerPolicy : IPolicy). The Polly Roadmap discusses whether we are ready to add that IPolicy interface (we have not yet for good reasons, tho getting closer).

This coupling could perhaps be unpicked by renaming the interface you propose something like ICircuitState, but do Isolate() and Reset() fit in that?; and is pulling it out of ICircuitBreakerPolicy desirable? (Comment welcome!)

I am going to recommend not merging this one immediately, because we need I think to resolve some of above questions first. But it's very likely we could do something like this, once/assuming IPolicy goes in. Furthermore, I think we'd want to extend the concept the PR proposes more widely, across all policies and variants: eg BulkheadAvailableCount on IBulkheadPolicy; LastHandledResult on an ICircuitBreakerPolicy<TResult> : ICircuitBreakerPolicy; etc.

Thanks again for the PR, which sheds light on an important need we can feed into the interface thinking.
(ie unifying between Policy and Policy<TResult>, and between different TResult forms generally)

@christopherbahr
Copy link
Author

Sorry to just drive-by PR you like that. I had all the best intentions of coming back with a more comprehensive solution but I never quite had the time to actually do it properly. I'll go ahead and close this. That new proposal looks great 👍

@christopherbahr christopherbahr deleted the adsk-contrib/AddCircuitBreakerPolicyInterface branch June 12, 2017 16:43
@reisenberger
Copy link
Member

@christopherbahr no worries! Thanks for the original suggestion, which definitely helped focus how we should do #257 .

@reisenberger
Copy link
Member

@christopherbahr Polly v5.2.0 released, including your original idea from here

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

Successfully merging this pull request may close these issues.

4 participants