schedule: region fit replace does not make slice (#5644)

close #5645 Signed-off-by: bufferflies <[email protected]> Co-authored-by: Ti Chi Robot <[email protected]>
tikv · Nov 30, 2022 · b94d940 · b94d940
1 parent c5b8838
commit b94d940
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 34 deletions.
diff --git a/server/schedule/filter/filters.go b/server/schedule/filter/filters.go
@@ -616,7 +616,7 @@ func (f *ruleFitFilter) Source(_ *config.PersistOptions, _ *core.StoreInfo) *pla
 // RegionA:[1,2,3], move peer1 --> peer2 will not allow, because it's count not match the rule.
 // but transfer role peer1 --> peer2, it will support.
 func (f *ruleFitFilter) Target(options *config.PersistOptions, store *core.StoreInfo) *plan.Status {
-	if f.oldFit.Replace(f.srcStore, store, f.region) {
+	if f.oldFit.Replace(f.srcStore, store) {
 		return statusOK
 	}
 	return statusStoreNotMatchRule
@@ -670,7 +670,7 @@ func (f *ruleLeaderFitFilter) Target(_ *config.PersistOptions, store *core.Store
 		log.Warn("ruleLeaderFitFilter couldn't find peer on target Store", zap.Uint64("target-store", store.GetID()))
 		return statusStoreNotMatchRule
 	}
-	if f.oldFit.Replace(f.srcLeaderStoreID, store, f.region) {
+	if f.oldFit.Replace(f.srcLeaderStoreID, store) {
 		return statusOK
 	}
 	return statusStoreNotMatchRule

diff --git a/server/schedule/placement/fit.go b/server/schedule/placement/fit.go
@@ -18,13 +18,14 @@ import (
 	"math"
 	"math/bits"
 	"sort"
-	"sync"
 
 	"github.com/pingcap/kvproto/pkg/metapb"
 	"github.com/tikv/pd/pkg/syncutil"
 	"github.com/tikv/pd/server/core"
 )
 
+const replicaBaseScore = 100
+
 // RegionFit is the result of fitting a region's peers to rule list.
 // All peers are divided into corresponding rules according to the matching
 // rules, and the remaining Peers are placed in the OrphanPeers list.
@@ -54,7 +55,7 @@ func (f *RegionFit) IsCached() bool {
 }
 
 // Replace return true if the replacement store is fit all constraints and isolation score is not less than the origin.
-func (f *RegionFit) Replace(srcStoreID uint64, dstStore *core.StoreInfo, region *core.RegionInfo) bool {
+func (f *RegionFit) Replace(srcStoreID uint64, dstStore *core.StoreInfo) bool {
 	fit := f.getRuleFitByStoreID(srcStoreID)
 	// check the target store is fit all constraints.
 	if fit == nil || !MatchLabelConstraints(dstStore, fit.Rule.LabelConstraints) {
@@ -66,11 +67,8 @@ func (f *RegionFit) Replace(srcStoreID uint64, dstStore *core.StoreInfo, region
 		return true
 	}
 
-	peers := newFitPeer(f.regionStores, region, fit.Peers, replaceFitPeerOpt(srcStoreID, dstStore))
-	score := isolationScore(peers, fit.Rule.LocationLabels)
-	for _, peer := range peers {
-		fitPeerPool.Put(peer)
-	}
+	score := isolationStoreScore(srcStoreID, dstStore, fit.stores, fit.Rule.LocationLabels)
+	// restore the source store.
 	return fit.IsolationScore <= score
 }
 
@@ -128,6 +126,8 @@ type RuleFit struct {
 	// IsolationScore indicates at which level of labeling these Peers are
 	// isolated. A larger value is better.
 	IsolationScore float64 `json:"isolation-score"`
+	// stores is the stores that the peers are placed in.
+	stores []*core.StoreInfo
 }
 
 // IsSatisfied returns if the rule is properly satisfied.
@@ -186,25 +186,13 @@ type fitWorker struct {
 	exit           bool
 }
 
-type fitPeerOpt func(peer *fitPeer)
-
-func replaceFitPeerOpt(srcStoreID uint64, dstStore *core.StoreInfo) fitPeerOpt {
-	return func(peer *fitPeer) {
-		if peer.Peer.GetStoreId() == srcStoreID {
-			peer.store = dstStore
-		}
-	}
-}
-
-func newFitPeer(stores []*core.StoreInfo, region *core.RegionInfo, fitPeers []*metapb.Peer, opts ...fitPeerOpt) []*fitPeer {
+func newFitPeer(stores []*core.StoreInfo, region *core.RegionInfo, fitPeers []*metapb.Peer) []*fitPeer {
 	peers := make([]*fitPeer, len(fitPeers))
 	for i, p := range fitPeers {
-		peer := fitPeerPool.Get().(*fitPeer)
-		peer.Peer = p
-		peer.store = getStoreByID(stores, p.GetStoreId())
-		peer.isLeader = region.GetLeader().GetId() == p.GetId()
-		for _, opt := range opts {
-			opt(peer)
+		peer := &fitPeer{
+			Peer:     p,
+			store:    getStoreByID(stores, p.GetStoreId()),
+			isLeader: region.GetLeader().GetId() == p.GetId(),
 		}
 		peers[i] = peer
 	}
@@ -366,6 +354,7 @@ func newRuleFit(rule *Rule, peers []*fitPeer, supportWitness bool) *RuleFit {
 	rf := &RuleFit{Rule: rule, IsolationScore: isolationScore(peers, rule.LocationLabels)}
 	for _, p := range peers {
 		rf.Peers = append(rf.Peers, p.Peer)
+		rf.stores = append(rf.stores, p.store)
 		if !p.matchRoleStrict(rule.Role) ||
 			(supportWitness && (p.IsWitness != rule.IsWitness)) ||
 			(!supportWitness && p.IsWitness) {
@@ -382,12 +371,6 @@ type fitPeer struct {
 	selected bool
 }
 
-var fitPeerPool = sync.Pool{
-	New: func() interface{} {
-		return new(fitPeer)
-	},
-}
-
 func (p *fitPeer) matchRoleStrict(role PeerRoleType) bool {
 	switch role {
 	case Voter: // Voter matches either Leader or Follower.
@@ -402,6 +385,28 @@ func (p *fitPeer) matchRoleStrict(role PeerRoleType) bool {
 	return false
 }
 
+func isolationStoreScore(srcStoreID uint64, dstStore *core.StoreInfo, stores []*core.StoreInfo, labels []string) float64 {
+	var score float64
+	if len(labels) == 0 || len(stores) <= 1 {
+		return 0
+	}
+	for i := range stores {
+		store1 := stores[i]
+		if store1.GetID() == srcStoreID {
+			store1 = dstStore
+		}
+		for _, store2 := range stores[i+1:] {
+			if store2.GetID() == srcStoreID {
+				store2 = dstStore
+			}
+			if index := store1.CompareLocation(store2, labels); index != -1 {
+				score += math.Pow(replicaBaseScore, float64(len(labels)-index-1))
+			}
+		}
+	}
+	return score
+}
+
 func isolationScore(peers []*fitPeer, labels []string) float64 {
 	var score float64
 	if len(labels) == 0 || len(peers) <= 1 {
@@ -413,7 +418,6 @@ func isolationScore(peers []*fitPeer, labels []string) float64 {
 	// here because it is kind of hot path.
 	// After Go supports generics, we will be enable to do some refactor and
 	// reuse `core.DistinctScore`.
-	const replicaBaseScore = 100
 	for i, p1 := range peers {
 		for _, p2 := range peers[i+1:] {
 			if index := p1.store.CompareLocation(p2.store, labels); index != -1 {

diff --git a/server/schedule/placement/fit_test.go b/server/schedule/placement/fit_test.go
@@ -147,7 +147,7 @@ func TestReplace(t *testing.T) {
 		}
 		rf := fitRegion(stores.GetStores(), region, rules, false)
 		rf.regionStores = stores.GetStores()
-		re.Equal(rf.Replace(tc.srcStoreID, stores.GetStore(tc.dstStoreID), region), tc.ok)
+		re.Equal(rf.Replace(tc.srcStoreID, stores.GetStore(tc.dstStoreID)), tc.ok)
 	}
 }