checker: merge-region should consider the peer on an offline store (t…

…ikv#4357) * merge-region should not consider the region on a deleted store (close tikv#4119) Signed-off-by: JmPotato <[email protected]> * Check whether the store is offline Signed-off-by: JmPotato <[email protected]> * Address the comment Signed-off-by: JmPotato <[email protected]> * Address the comment Signed-off-by: JmPotato <[email protected]> Co-authored-by: Ti Chi Robot <[email protected]>
IcePigZDB · Nov 29, 2021 · 9fd0246 · 9fd0246
1 parent aeaaf01
commit 9fd0246
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 5 deletions.
diff --git a/server/schedule/checker/merge_checker.go b/server/schedule/checker/merge_checker.go
@@ -111,7 +111,7 @@ func (m *MergeChecker) Check(region *core.RegionInfo) []*operator.Operator {
 		return nil
 	}
 
-	// skip region has down peers or pending peers or learner peers
+	// skip region has down peers or pending peers
 	if !opt.IsRegionHealthy(region) {
 		checkerCounter.WithLabelValues("merge_checker", "special-peer").Inc()
 		return nil
@@ -168,12 +168,12 @@ func (m *MergeChecker) Check(region *core.RegionInfo) []*operator.Operator {
 
 func (m *MergeChecker) checkTarget(region, adjacent *core.RegionInfo) bool {
 	return adjacent != nil && !m.splitCache.Exists(adjacent.GetID()) && !m.cluster.IsRegionHot(adjacent) &&
-		AllowMerge(m.cluster, region, adjacent) && opt.IsRegionHealthy(adjacent) &&
-		opt.IsRegionReplicated(m.cluster, adjacent)
+		AllowMerge(m.cluster, region, adjacent) && checkPeerStore(m.cluster, region, adjacent) &&
+		opt.IsRegionHealthy(adjacent) && opt.IsRegionReplicated(m.cluster, adjacent)
 }
 
 // AllowMerge returns true if two regions can be merged according to the key type.
-func AllowMerge(cluster opt.Cluster, region *core.RegionInfo, adjacent *core.RegionInfo) bool {
+func AllowMerge(cluster opt.Cluster, region, adjacent *core.RegionInfo) bool {
 	var start, end []byte
 	if bytes.Equal(region.GetEndKey(), adjacent.GetStartKey()) && len(region.GetEndKey()) != 0 {
 		start, end = region.GetStartKey(), adjacent.GetEndKey()
@@ -222,6 +222,23 @@ func AllowMerge(cluster opt.Cluster, region *core.RegionInfo, adjacent *core.Reg
 	}
 }
 
-func isTableIDSame(region *core.RegionInfo, adjacent *core.RegionInfo) bool {
+func isTableIDSame(region, adjacent *core.RegionInfo) bool {
 	return codec.Key(region.GetStartKey()).TableID() == codec.Key(adjacent.GetStartKey()).TableID()
 }
+
+// Check whether there is a peer of the adjacent region on an offline store,
+// while the source region has no peer on it. This is to prevent from bringing
+// any other peer into an offline store to slow down the offline process.
+func checkPeerStore(cluster opt.Cluster, region, adjacent *core.RegionInfo) bool {
+	regionStoreIDs := region.GetStoreIds()
+	for _, peer := range adjacent.GetPeers() {
+		storeID := peer.GetStoreId()
+		store := cluster.GetStore(storeID)
+		if store == nil || store.IsOffline() {
+			if _, ok := regionStoreIDs[storeID]; !ok {
+				return false
+			}
+		}
+	}
+	return true
+}
diff --git a/server/schedule/checker/merge_checker_test.go b/server/schedule/checker/merge_checker_test.go
@@ -163,6 +163,58 @@ func (s *testMergeCheckerSuite) TestBasic(c *C) {
 	c.Assert(ops[0].RegionID(), Equals, s.regions[2].GetID())
 	c.Assert(ops[1].RegionID(), Equals, s.regions[1].GetID())
 
+	// Test the peer store check.
+	store := s.cluster.GetStore(1)
+	c.Assert(store, NotNil)
+	// Test the peer store is deleted.
+	s.cluster.DeleteStore(store)
+	ops = s.mc.Check(s.regions[2])
+	c.Assert(ops, IsNil)
+	// Test the store is normal.
+	s.cluster.PutStore(store)
+	ops = s.mc.Check(s.regions[2])
+	c.Assert(ops, NotNil)
+	c.Assert(ops[0].RegionID(), Equals, s.regions[2].GetID())
+	c.Assert(ops[1].RegionID(), Equals, s.regions[1].GetID())
+	// Test the store is offline.
+	s.cluster.SetStoreOffline(store.GetID())
+	ops = s.mc.Check(s.regions[2])
+	// Only target region have a peer on the offline store,
+	// so it's not ok to merge.
+	c.Assert(ops, IsNil)
+	// Test the store is up.
+	s.cluster.SetStoreUp(store.GetID())
+	ops = s.mc.Check(s.regions[2])
+	c.Assert(ops, NotNil)
+	c.Assert(ops[0].RegionID(), Equals, s.regions[2].GetID())
+	c.Assert(ops[1].RegionID(), Equals, s.regions[1].GetID())
+	store = s.cluster.GetStore(5)
+	c.Assert(store, NotNil)
+	// Test the peer store is deleted.
+	s.cluster.DeleteStore(store)
+	ops = s.mc.Check(s.regions[2])
+	c.Assert(ops, IsNil)
+	// Test the store is normal.
+	s.cluster.PutStore(store)
+	ops = s.mc.Check(s.regions[2])
+	c.Assert(ops, NotNil)
+	c.Assert(ops[0].RegionID(), Equals, s.regions[2].GetID())
+	c.Assert(ops[1].RegionID(), Equals, s.regions[1].GetID())
+	// Test the store is offline.
+	s.cluster.SetStoreOffline(store.GetID())
+	ops = s.mc.Check(s.regions[2])
+	// Both regions have peers on the offline store,
+	// so it's ok to merge.
+	c.Assert(ops, NotNil)
+	c.Assert(ops[0].RegionID(), Equals, s.regions[2].GetID())
+	c.Assert(ops[1].RegionID(), Equals, s.regions[1].GetID())
+	// Test the store is up.
+	s.cluster.SetStoreUp(store.GetID())
+	ops = s.mc.Check(s.regions[2])
+	c.Assert(ops, NotNil)
+	c.Assert(ops[0].RegionID(), Equals, s.regions[2].GetID())
+	c.Assert(ops[1].RegionID(), Equals, s.regions[1].GetID())
+
 	// Enable one way merge
 	s.cluster.SetEnableOneWayMerge(true)
 	ops = s.mc.Check(s.regions[2])