-
Notifications
You must be signed in to change notification settings - Fork 439
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
Using circuitbreaker.Rule to define slowRequest/errorRatio/errorCount Rule #205
Using circuitbreaker.Rule to define slowRequest/errorRatio/errorCount Rule #205
Conversation
Codecov Report
@@ Coverage Diff @@
## master #205 +/- ##
==========================================
- Coverage 44.02% 42.41% -1.61%
==========================================
Files 81 82 +1
Lines 4534 4406 -128
==========================================
- Hits 1996 1869 -127
+ Misses 2298 2297 -1
Partials 240 240
Continue to review full report at Codecov.
|
1e54526
to
812eaf2
Compare
core/circuitbreaker/rule.go
Outdated
} | ||
|
||
func (b *RuleBase) IsApplicable() error { | ||
if len(b.Resource) == 0 { | ||
func (r *Rule) isApplicable() error { |
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.
Export this function is user-friendly?
How do you think of it?
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.
Since the rule itself is not an interface anymore, maybe we could make it an exported util checking function (like IsValidFlowRule
in flow/rule_manager.go
) instead of a member function?
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.
make sense
812eaf2
to
53b6b93
Compare
core/circuitbreaker/rule_manager.go
Outdated
if r.Threshold < 0 { | ||
return errors.New("invalid Threshold") | ||
} | ||
if r.Strategy == SlowRequestRatio && (r.Threshold < 0.0 || r.Threshold > 1.0) { |
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.
r.Threshold < 0.0
is not needed as it has been checked before.
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.
updated
core/circuitbreaker/rule_manager.go
Outdated
if r.Strategy == SlowRequestRatio && (r.Threshold < 0.0 || r.Threshold > 1.0) { | ||
return errors.New("invalid slow request ratio threshold (valid range: [0.0, 1.0])") | ||
} | ||
if r.Strategy == ErrorRatio && (r.Threshold < 0.0 || r.Threshold > 1.0) { |
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.
Ditto
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.
updated
if int(r.Strategy) < int(SlowRequestRatio) || int(r.Strategy) > int(ErrorCount) { | ||
return errors.New("invalid Strategy") | ||
} | ||
if r.StatIntervalMs <= 0 { |
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.
retryTimeout should also be positive
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.
updated
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
Nice work. Thanks! |
Describe what this PR does / why we need it
Currently, there are three rules to define circuit breaker rule: slowRequest/errorRatio/errorCount Rule.
The rule definition of circuit breaker rule is complex and there are also limitations to scalability.
So this PR will consolidate these circuit breaker rules to circuitbreaker.Rule.
Does this pull request fix one issue?
Describe how you did it
Describe how to verify it
Special notes for reviews