Issue13 circuit breaker implementation #18

louyuting · 2020-01-01T07:28:42Z

Describe what this PR does / why we need it

Add circuit breaker implementation. Including circuitBreaker、breakerRule、breakerManager.

Does this pull request fix one issue?

Fixes #13

sczyh30 · 2020-01-07T07:43:24Z

core/base/rule.go

@@ -6,4 +6,6 @@ type SentinelRule interface {
 	fmt.Stringer

 	ResourceName() string
+
+	PassCheck(ctx *EntryContext, node StatNode, AcquireCount uint32) bool


In my opinion, the rule entity per se should not do rule checking. Wrapping the checking logic (and the rule entity itself) in the corresponding rule checker (i.e. TrafficShapingController, CircuitBreaker) might be better.

Thanks for your advice. I would decouple the checking logic and wrap related logic in CircuitBreaker.

sczyh30 · 2020-01-07T07:46:49Z

core/circuitbreaker/rule.go

+}
+
+// The base struct of circuit breaker rule
+type BreakerRuleBase struct {


Maybe the rule entity should be separated from the de-facto "CircuitBreaker"?

sczyh30 · 2020-01-07T07:55:53Z

core/circuitbreaker/rule.go

+	TimeSilent   int64        `json:"timeSilent"`
+	SampleCount  uint32       `json:"sampleCount"`
+	IntervalInMs uint32       `json:"intervalInMs"`
+	Stat         sbase.Metric `json:"stat,omitempty"`


Actually using the original Metric here is not enough (but okay for legacy rules). For circuit breaking we need specialized data structures for the target metric type (e.g. RT or error ratio) which support arbitrary statistic time span (maybe not backed by sliding window).

For MPV, will only support some simple circuit breaker. Based on my design, we could add other robust and accurate circuit breaker in the future.

sczyh30 · 2020-01-07T07:57:42Z

core/circuitbreaker/rule_manager.go

+	}
+)
+
+type RuleManager struct {


Is it necessary to have a RuleManager wrapper here?

sczyh30 · 2020-03-07T15:40:52Z

core/circuit_breaker/rule.go

+	// return the strategy type
+	BreakerStrategy() BreakerStrategy
+
+	isApplicable() bool


What does "applicable" mean? valid?

What does "applicable" mean? valid?

Valid and can be converted to circuit breaker.

sczyh30 · 2020-03-07T15:43:10Z

core/circuit_breaker/rule.go

+	// resource name
+	Resource string          `json:"resource"`
+	Strategy BreakerStrategy `json:"strategy"`
+	// silent status, all requests would be broken during silent phase.


In circuit breaking it's often called a timeout period.

sczyh30 · 2020-03-07T15:44:58Z

core/circuit_breaker/rule.go

+)
+
+// The base interface of circuit breaker rule
+type BreakerRule interface {


Maybe it's better to be a CircuitBreakerRule? (or just Rule according to the convention of Go)

Maybe it's better to be a CircuitBreakerRule? (or just Rule according to the convention of Go)

Just Rule might make sense.
package name has indicated the circuit_breaker.

sczyh30 · 2020-03-07T15:45:54Z

core/circuit_breaker/rule.go

+func (b *BreakerRuleBase) String() string {
+	r, err := json.Marshal(b)
+	if err != nil {
+		// feedback string


fallback string?

Yes, I will fix here

sczyh30 · 2020-03-07T15:47:32Z

core/circuit_breaker/rule.go

+	// the threshold of rt(ms)
+	Threshold float64 `json:"threshold"`
+	// the count of request exceed the threshold
+	PassCount int64 `json:"pass"`


The passCount is real-time stat of the generated circuit breaker, not the rule entity, so maybe it's not needed in the rule entity?

Yes, I would fix here.

sczyh30 · 2020-03-07T15:54:51Z

core/circuit_breaker/circuit_breaker.go

+type CircuitBreaker interface {
+	getRule() BreakerRule
+
+	CanPass(ctx *base.EntryContext, node base.StatNode, acquireCount uint32) *base.TokenResult


In fact this is a legacy bad design in Java versions. We may improve the interface later to make it a real "circuit breaker":)

In fact this is a legacy bad design in Java versions. We may improve the interface later to make it a real "circuit breaker":)

Make sense.
How about rename to "CheckBreaker"?

Using CircuitBreaker is okay. We may improve the interface later :)

sczyh30 · 2020-03-07T16:02:25Z

core/circuit_breaker/circuit_breaker.go

+
+func newErrorRatioCircuitBreaker(rule *errorRatioBreakerRule) *errorRatioCircuitBreaker {
+	resNode := stat.GetResourceNode(rule.Resource)
+	metric := resNode.GetOrCreateSlidingWindowMetric(rule.SampleCount, rule.IntervalInMs)


What if the resNode is nil? (i.e. when the resource has not been visited yet)

Note: This problem could be eliminated in later versions, as we're planning to create individual stat structures for circuit breakers, rather than use the universal ResourceNode.

sczyh30 · 2020-04-08T10:00:50Z

Could you please resolve the conflicts?

sczyh30

LGTM, we could improve the design later

louyuting force-pushed the Issue13-circuitbreaker-slot branch 3 times, most recently from 8919dc1 to f6cdcb8 Compare January 3, 2020 09:45

sczyh30 reviewed Jan 7, 2020

View reviewed changes

sczyh30 added the to-review PRs to review label Jan 7, 2020

louyuting force-pushed the Issue13-circuitbreaker-slot branch from f6cdcb8 to 647d431 Compare January 11, 2020 09:14

louyuting mentioned this pull request Jan 11, 2020

[Feature] Data-source and data property listener #7

Closed

louyuting changed the title ~~Issue13-circuitbreaker-slot~~ Issue13 circuit breaker slot and general property model Jan 11, 2020

louyuting mentioned this pull request Jan 17, 2020

[BUG] Data-Race for metricBucket.addRt #26

Closed

louyuting force-pushed the Issue13-circuitbreaker-slot branch 3 times, most recently from b1504e7 to b0c5688 Compare January 20, 2020 03:56

louyuting changed the title ~~Issue13 circuit breaker slot and general property model~~ Issue13 circuit breaker implement Jan 20, 2020

louyuting force-pushed the Issue13-circuitbreaker-slot branch from b0c5688 to 7c9478c Compare January 20, 2020 14:03

louyuting requested a review from sczyh30 January 20, 2020 14:06

louyuting removed the to-review PRs to review label Feb 12, 2020

sczyh30 added area/circuit-breaking Issues or PRs related to circuit breaking to-review PRs to review labels Feb 12, 2020

louyuting force-pushed the Issue13-circuitbreaker-slot branch from 7c9478c to 65a8eaf Compare February 13, 2020 11:05

louyuting removed the to-review PRs to review label Feb 13, 2020

louyuting added this to the 0.2.0 milestone Feb 13, 2020

louyuting force-pushed the Issue13-circuitbreaker-slot branch from 65a8eaf to f640e4b Compare February 13, 2020 13:49

louyuting removed the request for review from sczyh30 February 15, 2020 06:25

louyuting self-assigned this Feb 15, 2020

louyuting changed the title ~~Issue13 circuit breaker implement~~ Issue13 circuit breaker implementation Feb 15, 2020

louyuting force-pushed the Issue13-circuitbreaker-slot branch 4 times, most recently from b96b0ff to bd39224 Compare February 18, 2020 17:02

louyuting added the to-review PRs to review label Feb 19, 2020

louyuting force-pushed the Issue13-circuitbreaker-slot branch from afb4f3c to 4357d97 Compare February 20, 2020 15:04

sczyh30 reviewed Mar 7, 2020

View reviewed changes

louyuting force-pushed the Issue13-circuitbreaker-slot branch from 4357d97 to e33e54d Compare March 8, 2020 08:23

louyuting requested a review from sczyh30 March 8, 2020 14:09

louyuting force-pushed the Issue13-circuitbreaker-slot branch from f7e0555 to 49f6b77 Compare March 15, 2020 14:48

sczyh30 removed this from the 0.2.0 milestone Mar 17, 2020

louyuting force-pushed the Issue13-circuitbreaker-slot branch from 49f6b77 to 6243260 Compare March 23, 2020 16:37

louyuting added this to the 0.3.0 milestone Mar 23, 2020

louyuting force-pushed the Issue13-circuitbreaker-slot branch from 6243260 to 2f3e11c Compare April 8, 2020 11:02

circuit breaker implementation

cd9dfff

louyuting force-pushed the Issue13-circuitbreaker-slot branch from 2f3e11c to cd9dfff Compare April 8, 2020 11:08

sczyh30 approved these changes Apr 8, 2020

View reviewed changes

sczyh30 merged commit caa4e5f into alibaba:master Apr 8, 2020

sczyh30 removed the to-review PRs to review label Apr 8, 2020

Issue13 circuit breaker implementation #18

Issue13 circuit breaker implementation #18

Uh oh!

Conversation

louyuting commented Jan 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe what this PR does / why we need it

Does this pull request fix one issue?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sczyh30 commented Apr 8, 2020

Uh oh!

sczyh30 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

louyuting commented Jan 1, 2020 •

edited

Loading