Skip to content

percentage shouldn't go beyond 100 to keep the circuits closed on 100% error rate#2048

Open
bhaskar-singhal wants to merge 1 commit into
Netflix:masterfrom
bhaskar-singhal:circuit-breaker-error-threshold-at-100
Open

percentage shouldn't go beyond 100 to keep the circuits closed on 100% error rate#2048
bhaskar-singhal wants to merge 1 commit into
Netflix:masterfrom
bhaskar-singhal:circuit-breaker-error-threshold-at-100

Conversation

@bhaskar-singhal

@bhaskar-singhal bhaskar-singhal commented Sep 14, 2023

Copy link
Copy Markdown

circuitBreakerErrorThresholdPercentage is a number but as per the name it should be treated like a percentage. Defining a percentage over 100 doesn't make sense.
So if someone is defining this property as 100, that person would be expecting circuits to not open at all. We do have other properties with which we can keep the circuit forcefully closed/open but many teams write wrapper over hystrix to only let devs access the basic ones and it is expected the basic properties will suffice for all different scenarios.

We were trying to keep the circuits closed at 100% failure rate using circuitBreakerErrorThresholdPercentage and we failed. We didn't want to make circuitBreakerErrorThresholdPercentage 101 and then check for the circuit remaining closed as a percentage over 100 didn't sound about right.
Can we please see if this change makes sense?

@bhaskar-singhal bhaskar-singhal changed the title percentage shouldn't go beyond 100 percentage shouldn't go beyond 100 to keep the circuits closed on 100% error rate Sep 14, 2023
@ThinkingMan007

ThinkingMan007 commented Sep 14, 2023 via email

Copy link
Copy Markdown

@bhaskar-singhal

Copy link
Copy Markdown
Author

@jsoref @ThinkingMan007 please check

@jsoref

jsoref commented Sep 18, 2023

Copy link
Copy Markdown
Contributor

I'm not a maintainer, but this project has tests... https://github.com/Netflix/Hystrix/tree/master/hystrix-core/src/test I'd expect anything that's changing its behavior to be accompanied by either a change to an existing test or the introduction of a new test.

@ThinkingMan007

ThinkingMan007 commented Oct 3, 2023 via email

Copy link
Copy Markdown

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