Address comment

Signed-off-by: ZenoTan <[email protected]>
nolouch · Oct 12, 2020 · 0c935f9 · 0c935f9
1 parent 2d1adee
commit 0c935f9
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 6 deletions.
diff --git a/server/schedule/checker/learner_checker.go b/server/schedule/checker/learner_checker.go
@@ -36,14 +36,15 @@ func NewLearnerChecker(cluster opt.Cluster) *LearnerChecker {
 // Check verifies a region's role, creating an Operator if need.
 func (l *LearnerChecker) Check(region *core.RegionInfo) *operator.Operator {
 	for _, p := range region.GetLearners() {
+		//if region.GetPendingLearner(p.GetId()) != nil {
+		//	continue
+		//}
 		op, err := operator.CreatePromoteLearnerOperator("promote-learner", l.cluster, region, p)
 		if err != nil {
 			log.Debug("fail to create promote learner operator", errs.ZapError(err))
 			return nil
 		}
-		if op != nil {
-			return op
-		}
+		return op
 	}
 	return nil
 }
diff --git a/server/schedule/operator/builder.go b/server/schedule/operator/builder.go
@@ -45,6 +45,7 @@ type Builder struct {
 
 	// operation record
 	originPeers         peersMap
+	pendingPeers        peersMap
 	originLeaderStoreID uint64
 	targetPeers         peersMap
 	targetLeaderStoreID uint64
@@ -95,6 +96,7 @@ func NewBuilder(desc string, cluster opt.Cluster, region *core.RegionInfo, opts
 	// origin peers
 	err := b.err
 	originPeers := newPeersMap()
+	pendingPeers := newPeersMap()
 
 	for _, p := range region.GetPeers() {
 		if p == nil || p.GetStoreId() == 0 {
@@ -104,6 +106,10 @@ func NewBuilder(desc string, cluster opt.Cluster, region *core.RegionInfo, opts
 		originPeers.Set(p)
 	}
 
+	for _, p := range region.GetPendingPeers() {
+		pendingPeers.Set(p)
+	}
+
 	// origin leader
 	originLeaderStoreID := region.GetLeader().GetStoreId()
 	if _, ok := originPeers[originLeaderStoreID]; err == nil && !ok {
@@ -132,6 +138,7 @@ func NewBuilder(desc string, cluster opt.Cluster, region *core.RegionInfo, opts
 
 	b.rules = rules
 	b.originPeers = originPeers
+	b.pendingPeers = pendingPeers
 	b.originLeaderStoreID = originLeaderStoreID
 	b.targetPeers = originPeers.Copy()
 	b.allowDemote = supportJointConsensus
@@ -182,6 +189,8 @@ func (b *Builder) PromoteLearner(storeID uint64) *Builder {
 		b.err = errors.Errorf("cannot promote peer %d: not found", storeID)
 	} else if !core.IsLearner(peer) {
 		b.err = errors.Errorf("cannot promote peer %d: is not learner", storeID)
+	} else if _, ok := b.pendingPeers[storeID]; ok {
+		b.err = errors.Errorf("cannot promote peer %d: pending", storeID)
 	} else {
 		b.targetPeers.Set(&metapb.Peer{
 			Id:      peer.GetId(),

diff --git a/server/schedule/operator/builder_test.go b/server/schedule/operator/builder_test.go
@@ -442,3 +442,13 @@ func (s *testBuilderSuite) TestBuild(c *C) {
 		}
 	}
 }
+
+// Test for issue 3039
+func (s *testBuilderSuite) TestPromotePending(c *C) {
+	p := &metapb.Peer{Id: 2, StoreId: 2, Role: metapb.PeerRole_Learner}
+	region := core.NewRegionInfo(&metapb.Region{Id: 1, Peers: []*metapb.Peer{{Id: 1, StoreId: 1},
+		p}}, &metapb.Peer{Id: 1, StoreId: 1}, core.WithPendingPeers([]*metapb.Peer{p}))
+	builder := NewBuilder("test", s.cluster, region)
+	builder.PromoteLearner(2)
+	c.Assert(builder.err, NotNil)
+}
diff --git a/server/schedule/operator/create_operator.go b/server/schedule/operator/create_operator.go
@@ -34,9 +34,6 @@ func CreateAddPeerOperator(desc string, cluster opt.Cluster, region *core.Region
 
 // CreatePromoteLearnerOperator creates an operator that promotes a learner.
 func CreatePromoteLearnerOperator(desc string, cluster opt.Cluster, region *core.RegionInfo, peer *metapb.Peer) (*Operator, error) {
-	if region.GetPendingPeer(peer.GetId()) != nil {
-		return nil, nil
-	}
 	return NewBuilder(desc, cluster, region).
 		PromoteLearner(peer.GetStoreId()).
 		Build(0)