fix circuit breaker half-open state issue. #202
Conversation
Codecov Report
@@ Coverage Diff @@
## master #202 +/- ##
==========================================
+ Coverage 43.44% 44.02% +0.57%
==========================================
Files 81 81
Lines 4507 4534 +27
==========================================
+ Hits 1958 1996 +38
+ Misses 2319 2298 -21
- Partials 230 240 +10
Continue to review full report at Codecov.
|
louyuting
force-pushed
the
2020-08-11-circuit-breaker-fix
branch
from
44ecfca to
f135f63
Compare
louyuting
added
area/circuit-breaking
louyuting
force-pushed
the
2020-08-11-circuit-breaker-fix
branch
from
9012e9a to
d8bf782
Compare
… but the request is blocked by other circuit breaker or check slot, the state of probing circuit breaker don't rollback the state.
louyuting
force-pushed
the
2020-08-11-circuit-breaker-fix
branch
from
be5386e to
2c32884
Compare
| if options.err != nil { | ||
| ctx.SetError(options.err) | ||
| } | ||
| e.exitCtl.Do(func() { | ||
| for _, handler := range e.exitHandlers { |
There was a problem hiding this comment.
Do we need to handle "panic" from exit handlers?
There was a problem hiding this comment.
I will add defer to handle panic and move context "Refurbish" to defer func
| @@ -87,8 +87,9 @@ type CircuitBreaker interface { | |||
| TryPass(ctx *base.EntryContext) bool | |||
| // CurrentState returns current state of the circuit breaker. | |||
| CurrentState() State | |||
| // OnRequestComplete record a completed request with the given response time as well as error (if present), | |||
| // OnRequestComplete record a passed request with the given response time as well as error (if present), | |||
| if entry == nil { | ||
| logging.Errorf("nil entry when probing, rule: %+v", b.rule) | ||
| } else { | ||
| entry.WhenExit(func(entry *base.SentinelEntry, ctx *base.EntryContext) error { |
There was a problem hiding this comment.
Could you please add some descriptions here?
| @@ -436,9 +453,11 @@ func (b *errorRatioCircuitBreaker) OnRequestComplete(rt uint64, err error) { | |||
| } | |||
| if curStatus == HalfOpen { | |||
| if err == nil { | |||
| logging.Debugf("probe successful, rule: %+v", b.BoundRule()) | |||
There was a problem hiding this comment.
Are these debug logs needed? Actually we may add a state change observer to achieve this.
There was a problem hiding this comment.
Just for debug when no state change observer
There was a problem hiding this comment.
Maybe register state change observer make more sense
| @@ -79,7 +79,7 @@ func (b *RuleBase) IsApplicable() error { | |||
| if b.RetryTimeoutMs <= 0 { | |||
| return errors.New("invalid RetryTimeoutMs") | |||
| } | |||
| if b.MinRequestAmount <= 0 { | |||
| if b.MinRequestAmount < 0 { | |||
There was a problem hiding this comment.
Maybe 0 does not make sense here? (at least 1)
There was a problem hiding this comment.
Maybe it's up to the user here?
| @@ -284,6 +284,10 @@ func RegisterStateChangeListeners(listeners ...StateChangeListener) { | |||
| stateChangeListeners = append(stateChangeListeners, listeners...) | |||
| } | |||
|
|
|||
| func ClearStateChangeListeners() { | |||
| stateChangeListeners = make([]StateChangeListener, 0) | |||
There was a problem hiding this comment.
We may need to keep the behavior consistent with RegisterStateChangeListeners regarding thread-safety. Actually this is not required to be thread-safe (describing it in comments is okay), as we didn't use read-lock when accessing the state change observers.
There was a problem hiding this comment.
Yes, it's not thread-safety.
But i think this function is meaningful,
I will add comments upon func
|
Nice work. Thanks for contributing! |
Describe what this PR does / why we need it
Fix the issue: #196
add defer to handle panic in entry.Exit func and move context recovery logic to defer
Does this pull request fix one issue?
#196
Describe how you did it
Describe how to verify it
UT and integration test
tests/core/circuitbreaker/circuitbreaker_slot_integration_test.go
Special notes for reviews