Refine GetRulesOfResource and GetRules API in all kinds of rule managers (#239)

louyuting · web-flow · commit 5250310370fc · 2020-09-14T22:47:16.000+08:00
diff --git a/core/circuitbreaker/rule_manager.go b/core/circuitbreaker/rule_manager.go
@@ -69,12 +69,35 @@ func init() {
 	}
 }
 
-func GetResRules(resource string) []*Rule {
+// GetRulesOfResource returns specific resource's rules based on copy.
+// It doesn't take effect for circuit breaker module if user changes the rule.
+// GetRulesOfResource need to compete circuit breaker module's global lock and the high performance losses of copy,
+// 		reduce or do not call GetRulesOfResource frequently if possible
+func GetRulesOfResource(resource string) []Rule {
 	updateMux.RLock()
-	ret, ok := breakerRules[resource]
+	resRules, ok := breakerRules[resource]
 	updateMux.RUnlock()
 	if !ok {
-		ret = make([]*Rule, 0)
+		return nil
+	}
+	ret := make([]Rule, 0, len(resRules))
+	for _, rule := range resRules {
+		ret = append(ret, *rule)
+	}
+	return ret
+}
+
+// GetRules returns all the rules based on copy.
+// It doesn't take effect for circuit breaker module if user changes the rule.
+// GetRules need to compete circuit breaker module's global lock and the high performance losses of copy,
+// 		reduce or do not call GetRules if possible
+func GetRules() []Rule {
+	updateMux.RLock()
+	rules := rulesFrom(breakerRules)
+	updateMux.RUnlock()
+	ret := make([]Rule, 0, len(rules))
+	for _, rule := range rules {
+		ret = append(ret, *rule)
 	}
 	return ret
 }
@@ -97,7 +120,7 @@ func LoadRules(rules []*Rule) (bool, error) {
 	return true, err
 }
 
-func getResBreakers(resource string) []CircuitBreaker {
+func getBreakersOfResource(resource string) []CircuitBreaker {
 	ret := make([]CircuitBreaker, 0)
 	updateMux.RLock()
 	resCBs := breakers[resource]
diff --git a/core/circuitbreaker/slot.go b/core/circuitbreaker/slot.go
@@ -24,7 +24,7 @@ func (b *Slot) Check(ctx *base.EntryContext) *base.TokenResult {
 }
 
 func checkPass(ctx *base.EntryContext) (bool, *Rule) {
-	breakers := getResBreakers(ctx.Resource.Name())
+	breakers := getBreakersOfResource(ctx.Resource.Name())
 	for _, breaker := range breakers {
 		passed := breaker.TryPass(ctx)
 		if !passed {
diff --git a/core/circuitbreaker/stat_slot.go b/core/circuitbreaker/stat_slot.go
@@ -23,7 +23,7 @@ func (c *MetricStatSlot) OnCompleted(ctx *base.EntryContext) {
 	res := ctx.Resource.Name()
 	err := ctx.Err()
 	rt := ctx.Rt()
-	for _, cb := range getResBreakers(res) {
+	for _, cb := range getBreakersOfResource(res) {
 		cb.OnRequestComplete(rt, err)
 	}
 }
diff --git a/core/flow/rule_manager.go b/core/flow/rule_manager.go
@@ -116,9 +116,9 @@ func getRules() []*Rule {
 	return rulesFrom(tcMap)
 }
 
-// getResRules returns specific resource's rules。Any changes of rules take effect for flow module
-// getResRules is an internal interface.
-func getResRules(res string) []*Rule {
+// getRulesOfResource returns specific resource's rules。Any changes of rules take effect for flow module
+// getRulesOfResource is an internal interface.
+func getRulesOfResource(res string) []*Rule {
 	tcMux.RLock()
 	defer tcMux.RUnlock()
 
@@ -144,10 +144,10 @@ func GetRules() []Rule {
 	return ret
 }
 
-// GetResRules returns specific resource's rules based on copy.
+// GetRulesOfResource returns specific resource's rules based on copy.
 // It doesn't take effect for flow module if user changes the rule.
-func GetResRules(res string) []Rule {
-	rules := getResRules(res)
+func GetRulesOfResource(res string) []Rule {
+	rules := getRulesOfResource(res)
 	ret := make([]Rule, 0, len(rules))
 	for _, rule := range rules {
 		ret = append(ret, *rule)
diff --git a/core/hotspot/rule_manager.go b/core/hotspot/rule_manager.go
@@ -68,14 +68,34 @@ func LoadRules(rules []*Rule) (bool, error) {
 	return true, err
 }
 
-// GetRules returns existing rules of the given resource.
-func GetRules(res string) []*Rule {
+// GetRules returns all the rules based on copy.
+// It doesn't take effect for hotspot module if user changes the rule.
+// GetRules need to compete hotspot module's global lock and the high performance losses of copy,
+// 		reduce or do not call GetRules if possible
+func GetRules() []Rule {
+	tcMux.RLock()
+	rules := rulesFrom(tcMap)
+	tcMux.RUnlock()
+
+	ret := make([]Rule, 0, len(rules))
+	for _, rule := range rules {
+		ret = append(ret, *rule)
+	}
+	return ret
+}
+
+// GetRulesOfResource returns specific resource's rules based on copy.
+// It doesn't take effect for hotspot module if user changes the rule.
+// GetRulesOfResource need to compete hotspot module's global lock and the high performance losses of copy,
+// 		reduce or do not call GetRulesOfResource frequently if possible
+func GetRulesOfResource(res string) []Rule {
 	tcMux.RLock()
-	defer tcMux.RUnlock()
 	resTcs := tcMap[res]
-	ret := make([]*Rule, 0, len(resTcs))
+	tcMux.RUnlock()
+
+	ret := make([]Rule, 0, len(resTcs))
 	for _, tc := range resTcs {
-		ret = append(ret, tc.BoundRule())
+		ret = append(ret, *tc.BoundRule())
 	}
 	return ret
 }
diff --git a/core/system/rule_manager.go b/core/system/rule_manager.go
@@ -16,8 +16,28 @@ var (
 	ruleMapMux = new(sync.RWMutex)
 )
 
-// GetRules return all the rules
-func GetRules() []*Rule {
+// GetRules returns all the rules based on copy.
+// It doesn't take effect for system module if user changes the rule.
+// GetRules need to compete system module's global lock and the high performance losses of copy,
+// 		reduce or do not call GetRules if possible
+func GetRules() []Rule {
+	rules := make([]*Rule, 0, len(ruleMap))
+	ruleMapMux.RLock()
+	for _, rs := range ruleMap {
+		rules = append(rules, rs...)
+	}
+	ruleMapMux.RUnlock()
+
+	ret := make([]Rule, 0, len(rules))
+	for _, r := range rules {
+		ret = append(ret, *r)
+	}
+	return ret
+}
+
+// getRules returns all the rules。Any changes of rules take effect for system module
+// getRules is an internal interface.
+func getRules() []*Rule {
 	ruleMapMux.RLock()
 	defer ruleMapMux.RUnlock()
 
diff --git a/core/system/rule_manager_test.go b/core/system/rule_manager_test.go
@@ -8,7 +8,7 @@ import (
 
 func TestGetRules(t *testing.T) {
 	t.Run("EmptyRules", func(t *testing.T) {
-		rules := GetRules()
+		rules := getRules()
 		assert.Equal(t, 0, len(rules))
 	})
 
@@ -20,12 +20,12 @@ func TestGetRules(t *testing.T) {
 			Concurrency: {&Rule{MetricType: Concurrency, TriggerCount: 2}},
 		}
 		ruleMap = r
-		rules := GetRules()
+		rules := getRules()
 		assert.Equal(t, 2, len(rules))
 
 		r[InboundQPS] = append(r[InboundQPS], &Rule{MetricType: InboundQPS, TriggerCount: 2})
 		ruleMap = r
-		rules = GetRules()
+		rules = getRules()
 		assert.Equal(t, 3, len(rules))
 	})
 }
diff --git a/core/system/slot.go b/core/system/slot.go
@@ -12,7 +12,7 @@ func (s *AdaptiveSlot) Check(ctx *base.EntryContext) *base.TokenResult {
 	if ctx == nil || ctx.Resource == nil || ctx.Resource.FlowType() != base.Inbound {
 		return nil
 	}
-	rules := GetRules()
+	rules := getRules()
 	result := ctx.RuleCheckResult
 	for _, rule := range rules {
 		passed, snapshotValue := s.doCheckRule(rule)
diff --git a/ext/datasource/helper_test.go b/ext/datasource/helper_test.go
@@ -322,10 +322,10 @@ func TestCircuitBreakerRulesUpdater(t *testing.T) {
 	err := CircuitBreakerRulesUpdater([]*cb.Rule{r1, r2, r3})
 	assert.True(t, err == nil)
 
-	rules := cb.GetResRules("abc")
-	assert.True(t, reflect.DeepEqual(rules[0], r1))
-	assert.True(t, reflect.DeepEqual(rules[1], r2))
-	assert.True(t, reflect.DeepEqual(rules[2], r3))
+	rules := cb.GetRulesOfResource("abc")
+	assert.True(t, reflect.DeepEqual(rules[0], *r1))
+	assert.True(t, reflect.DeepEqual(rules[1], *r2))
+	assert.True(t, reflect.DeepEqual(rules[2], *r3))
 }
 
 func TestHotSpotParamRuleListJsonConverter(t *testing.T) {
@@ -455,9 +455,9 @@ func TestHotSpotParamRuleListJsonUpdater(t *testing.T) {
 	err := HotSpotParamRulesUpdater([]*hotspot.Rule{r1, r2, r3, r4})
 	assert.True(t, err == nil)
 
-	rules := hotspot.GetRules("abc")
-	assert.True(t, rules[0].Equals(r1))
-	assert.True(t, rules[1].Equals(r2))
-	assert.True(t, rules[2].Equals(r3))
-	assert.True(t, rules[3].Equals(r4))
+	rules := hotspot.GetRulesOfResource("abc")
+	assert.True(t, reflect.DeepEqual(rules[0], *r1))
+	assert.True(t, reflect.DeepEqual(rules[1], *r2))
+	assert.True(t, reflect.DeepEqual(rules[2], *r3))
+	assert.True(t, reflect.DeepEqual(rules[3], *r4))
 }

Original file line number	Diff line number	Diff line change
`@@ -24,7 +24,7 @@ func (b Slot) Check(ctx base.EntryContext) *base.TokenResult {`
`24`	`24`	`}`
`25`	`25`
`26`	`26`	`func checkPass(ctx base.EntryContext) (bool, Rule) {`
`27`		`- breakers := getResBreakers(ctx.Resource.Name())`
	`27`	`+ breakers := getBreakersOfResource(ctx.Resource.Name())`
`28`	`28`	`for _, breaker := range breakers {`
`29`	`29`	`passed := breaker.TryPass(ctx)`
`30`	`30`	`if !passed {`
Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,7 @@ func (c MetricStatSlot) OnCompleted(ctx base.EntryContext) {`
`23`	`23`	`res := ctx.Resource.Name()`
`24`	`24`	`err := ctx.Err()`
`25`	`25`	`rt := ctx.Rt()`
`26`		`- for _, cb := range getResBreakers(res) {`
	`26`	`+ for _, cb := range getBreakersOfResource(res) {`
`27`	`27`	`cb.OnRequestComplete(rt, err)`
`28`	`28`	`}`
`29`	`29`	`}`
Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,7 @@ func (s AdaptiveSlot) Check(ctx base.EntryContext) *base.TokenResult {`
`12`	`12`	`if ctx == nil \|\| ctx.Resource == nil \|\| ctx.Resource.FlowType() != base.Inbound {`
`13`	`13`	`return nil`
`14`	`14`	`}`
`15`		`- rules := GetRules()`
	`15`	`+ rules := getRules()`
`16`	`16`	`result := ctx.RuleCheckResult`
`17`	`17`	`for _, rule := range rules {`
`18`	`18`	`passed, snapshotValue := s.doCheckRule(rule)`