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

Change AllowRequest logic to follow Netflix hystrix implementation #19

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sidai
Copy link

@sidai sidai commented Aug 19, 2022

Problem to resolve:

The current AllowRequest will perform a CAS on the openedOrLastTestedTime. Given a scenario where we want to perform a AllowRequest to determine if to use an external service before invoke the hystrix.Do/Go function for actual execution, once the circuit is open, the AllowRequest function in hystrix.Do/Go will always return ErrCircuitOpen since the openedOrLastTestedTime just been updated by the first AllowRequest call.

This can cause the circuit to always open without having chance to measure whether the external service has recovered.


Observation:

The Netflix circuit breaker actually provide allowRequest and attemptExecution function to resolve this case. Where the allowRequest won't change circuit status and is used externally to pre-check if a service call should be permitted before the actual execution. For attemptExecution, it is used internally right before the execution to ensure only the first request is let through after each sleep window.

image


Fix:

This MR follow a similar implementation to update the existing AllowRequest function which could be used externally and add a attemptExecution to be used internally right before the actually execution in hystrix.Do/Go. This allows combination usage of AllowRequest and hystrix.Do/Go.

…nd add AllowRequest to be used to check if a external service could be used
Copy link

@swamvenk swamvenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

2 participants