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
Allow only system failures to trigger circuit breaker #667
Conversation
When the downstream client call fails due to application errors, we should not count that towards the circuit breaker threshold. The systems are still behaving okay. For tchannel, it is the "system errors" For http downstream, we sorta count 5xx as system failures. This may not always be the case, but is there a better way? For grpc downstream, all exceptions are system errors, so things should be okay.
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.
besides the cosmetic change, LGTM.
codegen/templates/http_client.tmpl
Outdated
err = hystrix.DoC(ctx, "{{$clientID}}", func(ctx context.Context) error { | ||
res, err = req.Do() | ||
return err | ||
res, realErr = req.Do() |
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 would choose variable name as clientErr rather than realErr. real vs unreal it looks like from variable name currently
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.
LGTM
When the downstream client call fails due to application errors, we should not count that towards
the circuit breaker threshold. The systems are still behaving okay.
For tchannel, it is the "system errors"
For http downstream, we sorta count 5xx as system failures. This may not always be the case, but is there a better way?
For grpc downstream, all exceptions are system errors, so things should be okay.