statistics: fix the bug that the isolation level is wrong when the st…

…ore lacks label (#3467) * remove counterInc and counterDec Signed-off-by: HunDunDM <hundundm@gmail.com> * add unit test Signed-off-by: HunDunDM <hundundm@gmail.com> * statistics: fix the bug that the isolation level is wrong when the store lacks label Signed-off-by: HunDunDM <hundundm@gmail.com> * add unit test Signed-off-by: HunDunDM <hundundm@gmail.com> * add comment Signed-off-by: HunDunDM <hundundm@gmail.com>
tikv · Mar 12, 2021 · 3c81817 · 3c81817
1 parent 2cf8161
commit 3c81817
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 35 deletions.
diff --git a/server/cluster/cluster.go b/server/cluster/cluster.go
@@ -655,7 +655,7 @@ func (c *RaftCluster) processRegionHeartbeat(region *core.RegionInfo) error {
 			if c.regionStats != nil {
 				c.regionStats.ClearDefunctRegion(item.GetID())
 			}
-			c.labelLevelStats.ClearDefunctRegion(item.GetID(), c.opt.GetLocationLabels())
+			c.labelLevelStats.ClearDefunctRegion(item.GetID())
 		}
 
 		// Update related stores.

diff --git a/server/statistics/hot_peer_cache_test.go b/server/statistics/hot_peer_cache_test.go
@@ -15,18 +15,13 @@ package statistics
 
 import (
 	"math/rand"
-	"testing"
 	"time"
 
 	. "github.com/pingcap/check"
 	"github.com/pingcap/kvproto/pkg/metapb"
 	"github.com/tikv/pd/server/core"
 )
 
-func Test(t *testing.T) {
-	TestingT(t)
-}
-
 var _ = Suite(&testHotPeerCache{})
 
 type testHotPeerCache struct{}

diff --git a/server/statistics/region_collection.go b/server/statistics/region_collection.go
@@ -232,10 +232,10 @@ func (l *LabelStatistics) Observe(region *core.RegionInfo, stores []*core.StoreI
 		if label == regionIsolation {
 			return
 		}
-		l.counterDec(label)
+		l.labelCounter[label]--
 	}
 	l.regionLabelStats[regionID] = regionIsolation
-	l.counterInc(regionIsolation)
+	l.labelCounter[regionIsolation]++
 }
 
 // Collect collects the metrics of the label status.
@@ -251,26 +251,10 @@ func (l *LabelStatistics) Reset() {
 }
 
 // ClearDefunctRegion is used to handle the overlap region.
-func (l *LabelStatistics) ClearDefunctRegion(regionID uint64, labels []string) {
+func (l *LabelStatistics) ClearDefunctRegion(regionID uint64) {
 	if label, ok := l.regionLabelStats[regionID]; ok {
-		l.counterDec(label)
-		delete(l.regionLabelStats, regionID)
-	}
-}
-
-func (l *LabelStatistics) counterInc(label string) {
-	if label == nonIsolation {
-		l.labelCounter[nonIsolation]++
-	} else {
-		l.labelCounter[label]++
-	}
-}
-
-func (l *LabelStatistics) counterDec(label string) {
-	if label == nonIsolation {
-		l.labelCounter[nonIsolation]--
-	} else {
 		l.labelCounter[label]--
+		delete(l.regionLabelStats, regionID)
 	}
 }
 
@@ -296,17 +280,39 @@ func getRegionLabelIsolation(stores []*core.StoreInfo, labels []string) string {
 }
 
 func notIsolatedStoresWithLabel(stores []*core.StoreInfo, label string) [][]*core.StoreInfo {
-	m := make(map[string][]*core.StoreInfo)
+	var emptyValueStores []*core.StoreInfo
+	valueStoresMap := make(map[string][]*core.StoreInfo)
+
 	for _, s := range stores {
 		labelValue := s.GetLabelValue(label)
 		if labelValue == "" {
-			continue
+			emptyValueStores = append(emptyValueStores, s)
+		} else {
+			valueStoresMap[labelValue] = append(valueStoresMap[labelValue], s)
+		}
+	}
+
+	if len(valueStoresMap) == 0 {
+		// Usually it is because all TiKVs lack this label.
+		if len(emptyValueStores) > 1 {
+			return [][]*core.StoreInfo{emptyValueStores}
 		}
-		m[labelValue] = append(m[labelValue], s)
+		return nil
 	}
+
 	var res [][]*core.StoreInfo
-	for _, stores := range m {
-		if len(stores) > 1 {
+	if len(emptyValueStores) == 0 {
+		// No TiKV lacks this label.
+		for _, stores := range valueStoresMap {
+			if len(stores) > 1 {
+				res = append(res, stores)
+			}
+		}
+	} else {
+		// Usually it is because some TiKVs lack this label.
+		// The TiKVs in each label and the TiKVs without label form a group.
+		for _, stores := range valueStoresMap {
+			stores = append(stores, emptyValueStores...)
 			res = append(res, stores)
 		}
 	}

diff --git a/server/statistics/region_collection_test.go b/server/statistics/region_collection_test.go
@@ -217,9 +217,21 @@ func (t *testRegionStatisticsSuite) TestRegionLabelIsolationLevel(c *C) {
 			{"zone": "z1", "rack": "r2", "host": "h2"},
 			{"zone": "z1", "rack": "r2", "host": "h2"},
 		},
+		{
+			// isolated by rack
+			{"rack": "r1", "host": "h1"},
+			{"rack": "r2", "host": "h2"},
+			{"rack": "r3", "host": "h3"},
+		},
+		{
+			// isolated by host
+			{"zone": "z1", "rack": "r1", "host": "h1"},
+			{"zone": "z1", "rack": "r2", "host": "h2"},
+			{"zone": "z1", "host": "h3"},
+		},
 	}
-	res := []string{"rack", "host", "zone", "rack", "none"}
-	counter := map[string]int{"none": 1, "host": 1, "rack": 2, "zone": 1}
+	res := []string{"rack", "host", "zone", "rack", "none", "rack", "host"}
+	counter := map[string]int{"none": 1, "host": 2, "rack": 3, "zone": 1}
 	regionID := 1
 	f := func(labels []map[string]string, res string, locationLabels []string) {
 		metaStores := []*metapb.Store{
@@ -255,10 +267,13 @@ func (t *testRegionStatisticsSuite) TestRegionLabelIsolationLevel(c *C) {
 	c.Assert(label, Equals, nonIsolation)
 	label = getRegionLabelIsolation(nil, nil)
 	c.Assert(label, Equals, nonIsolation)
+	store := core.NewStoreInfo(&metapb.Store{Id: 1, Address: "mock://tikv-1"}, core.SetStoreLabels([]*metapb.StoreLabel{{Key: "foo", Value: "bar"}}))
+	label = getRegionLabelIsolation([]*core.StoreInfo{store}, locationLabels)
+	c.Assert(label, Equals, "zone")
 
 	regionID = 1
-	res = []string{"rack", "none", "zone", "rack", "none"}
-	counter = map[string]int{"none": 2, "host": 0, "rack": 2, "zone": 1}
+	res = []string{"rack", "none", "zone", "rack", "none", "rack", "none"}
+	counter = map[string]int{"none": 3, "host": 0, "rack": 3, "zone": 1}
 	locationLabels = []string{"zone", "rack"}
 
 	for i, labels := range labelsSet {