From 3c81817831e9c7890ab8807c93491193498e48fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B7=B7=E6=B2=8CDM?= Date: Fri, 12 Mar 2021 21:02:55 +0800 Subject: [PATCH] statistics: fix the bug that the isolation level is wrong when the store lacks label (#3467) * remove counterInc and counterDec Signed-off-by: HunDunDM * add unit test Signed-off-by: HunDunDM * statistics: fix the bug that the isolation level is wrong when the store lacks label Signed-off-by: HunDunDM * add unit test Signed-off-by: HunDunDM * add comment Signed-off-by: HunDunDM --- server/cluster/cluster.go | 2 +- server/statistics/hot_peer_cache_test.go | 5 -- server/statistics/region_collection.go | 56 ++++++++++++--------- server/statistics/region_collection_test.go | 23 +++++++-- 4 files changed, 51 insertions(+), 35 deletions(-) diff --git a/server/cluster/cluster.go b/server/cluster/cluster.go index 2fcc6051975..92b58c24427 100644 --- 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 index d88ecf3ac0b..b61b33a4347 100644 --- a/server/statistics/hot_peer_cache_test.go +++ b/server/statistics/hot_peer_cache_test.go @@ -15,7 +15,6 @@ package statistics import ( "math/rand" - "testing" "time" . "github.com/pingcap/check" @@ -23,10 +22,6 @@ import ( "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 index 890bf199e98..6828900236b 100644 --- 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 index 42b640f1e8b..24f7054de0f 100644 --- 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 {