pd-ctl: reduce duplicate params in grant hot region scheduler (#8673)

close #8672

Signed-off-by: lhy1024 <[email protected]>

pkg/schedule/schedulers/grant_hot_region.go

@@ -49,17 +49,14 @@ type grantHotRegionSchedulerConfig struct {
 	StoreLeaderID uint64   `json:"store-leader-id"`
 }
 
-func (conf *grantHotRegionSchedulerConfig) setStore(leaderID uint64, peers []uint64) bool {
+func (conf *grantHotRegionSchedulerConfig) setStore(leaderID uint64, peers []uint64) {
 	conf.Lock()
 	defer conf.Unlock()
-	ret := slice.AnyOf(peers, func(i int) bool {
-		return leaderID == peers[i]
-	})
-	if ret {
-		conf.StoreLeaderID = leaderID
-		conf.StoreIDs = peers
+	if !slice.Contains(peers, leaderID) {
+		peers = append(peers, leaderID)
 	}
-	return ret
+	conf.StoreLeaderID = leaderID
+	conf.StoreIDs = peers
 }
 
 func (conf *grantHotRegionSchedulerConfig) getStoreLeaderID() uint64 {
@@ -194,10 +191,7 @@ func (handler *grantHotRegionHandler) updateConfig(w http.ResponseWriter, r *htt
 		handler.rd.JSON(w, http.StatusBadRequest, errs.ErrBytesToUint64)
 		return
 	}
-	if !handler.config.setStore(leaderID, storeIDs) {
-		handler.rd.JSON(w, http.StatusBadRequest, errs.ErrSchedulerConfig)
-		return
-	}
+	handler.config.setStore(leaderID, storeIDs)
 
 	if err = handler.config.persist(); err != nil {
 		handler.config.setStoreLeaderID(0)

pkg/schedule/schedulers/init.go

@@ -216,9 +216,7 @@ func schedulersRegister() {
 				}
 				storeIDs = append(storeIDs, storeID)
 			}
-			if !conf.setStore(leaderID, storeIDs) {
-				return errs.ErrSchedulerConfig
-			}
+			conf.setStore(leaderID, storeIDs)
 			return nil
 		}
 	})

tools/pd-ctl/pdctl/command/scheduler.go

@@ -355,7 +355,7 @@ func NewSplitBucketSchedulerCommand() *cobra.Command {
 // NewGrantHotRegionSchedulerCommand returns a command to add a grant-hot-region-scheduler.
 func NewGrantHotRegionSchedulerCommand() *cobra.Command {
 	c := &cobra.Command{
-		Use: "grant-hot-region-scheduler <store_leader_id> <store_leader_id,store_peer_id_1,store_peer_id_2>",
+		Use: "grant-hot-region-scheduler <store_leader_id> <store_follower_id_1,store_follower_id_2>",
 		Short: `add a scheduler to grant hot region to fixed stores. 
 				Note: If there is balance-hot-region-scheduler running, please remove it first, otherwise grant-hot-region-scheduler will not work.`,
 		Run: addSchedulerForGrantHotRegionCommandFunc,

tools/pd-ctl/tests/scheduler/scheduler_test.go

@@ -559,11 +559,11 @@ func (suite *schedulerTestSuite) checkSchedulerConfig(cluster *pdTests.TestClust
 	re.Contains(echo, "Success!")
 }
 
-func (suite *schedulerTestSuite) TestGrantLeaderScheduler() {
-	suite.env.RunTestBasedOnMode(suite.checkGrantLeaderScheduler)
+func (suite *schedulerTestSuite) TestGrantHotRegionScheduler() {
+	suite.env.RunTestBasedOnMode(suite.checkGrantHotRegionScheduler)
 }
 
-func (suite *schedulerTestSuite) checkGrantLeaderScheduler(cluster *pdTests.TestCluster) {
+func (suite *schedulerTestSuite) checkGrantHotRegionScheduler(cluster *pdTests.TestCluster) {
 	re := suite.Require()
 	pdAddr := cluster.GetConfig().GetClientURL()
 	cmd := ctl.GetRootCmd()
@@ -617,7 +617,7 @@ func (suite *schedulerTestSuite) checkGrantLeaderScheduler(cluster *pdTests.Test
 		"evict-slow-store-scheduler": true,
 	})
 
-	checkSchedulerCommand(re, cmd, pdAddr, []string{"-u", pdAddr, "scheduler", "add", "grant-hot-region-scheduler", "1", "1,2,3"}, map[string]bool{
+	checkSchedulerCommand(re, cmd, pdAddr, []string{"-u", pdAddr, "scheduler", "add", "grant-hot-region-scheduler", "1", "2,3"}, map[string]bool{
 		"balance-region-scheduler":   true,
 		"balance-leader-scheduler":   true,
 		"grant-hot-region-scheduler": true,
@@ -631,14 +631,33 @@ func (suite *schedulerTestSuite) checkGrantLeaderScheduler(cluster *pdTests.Test
 		"store-leader-id": float64(1),
 	}
 	mustExec(re, cmd, []string{"-u", pdAddr, "scheduler", "config", "grant-hot-region-scheduler"}, &conf3)
-	re.Equal(expected3, conf3)
+	re.True(compareGrantHotRegionSchedulerConfig(expected3, conf3))
 
-	echo = mustExec(re, cmd, []string{"-u", pdAddr, "scheduler", "config", "grant-hot-region-scheduler", "set", "2", "1,2,3"}, nil)
+	echo = mustExec(re, cmd, []string{"-u", pdAddr, "scheduler", "config", "grant-hot-region-scheduler", "set", "2", "1,3"}, nil)
 	re.Contains(echo, "Success!")
 	expected3["store-leader-id"] = float64(2)
 	testutil.Eventually(re, func() bool {
 		mustExec(re, cmd, []string{"-u", pdAddr, "scheduler", "config", "grant-hot-region-scheduler"}, &conf3)
-		return reflect.DeepEqual(expected3, conf3)
+		return compareGrantHotRegionSchedulerConfig(expected3, conf3)
+	})
+
+	checkSchedulerCommand(re, cmd, pdAddr, []string{"-u", pdAddr, "scheduler", "remove", "grant-hot-region-scheduler"}, map[string]bool{
+		"balance-region-scheduler":   true,
+		"balance-leader-scheduler":   true,
+		"evict-slow-store-scheduler": true,
+	})
+
+	// use duplicate store id
+	checkSchedulerCommand(re, cmd, pdAddr, []string{"-u", pdAddr, "scheduler", "add", "grant-hot-region-scheduler", "3", "1,2,3"}, map[string]bool{
+		"balance-region-scheduler":   true,
+		"balance-leader-scheduler":   true,
+		"grant-hot-region-scheduler": true,
+		"evict-slow-store-scheduler": true,
+	})
+	expected3["store-leader-id"] = float64(3)
+	testutil.Eventually(re, func() bool {
+		mustExec(re, cmd, []string{"-u", pdAddr, "scheduler", "config", "grant-hot-region-scheduler"}, &conf3)
+		return compareGrantHotRegionSchedulerConfig(expected3, conf3)
 	})
 
 	// case 5: remove grant-hot-region-scheduler
@@ -980,3 +999,28 @@ func checkSchedulerCommand(re *require.Assertions, cmd *cobra.Command, pdAddr st
 		return true
 	})
 }
+
+func compareGrantHotRegionSchedulerConfig(expect, actual map[string]any) bool {
+	if expect["store-leader-id"] != actual["store-leader-id"] {
+		return false
+	}
+	expectStoreID := expect["store-id"].([]any)
+	actualStoreID := actual["store-id"].([]any)
+	if len(expectStoreID) != len(actualStoreID) {
+		return false
+	}
+	count := map[float64]any{}
+	for _, id := range expectStoreID {
+		// check if the store id is duplicated
+		if _, ok := count[id.(float64)]; ok {
+			return false
+		}
+		count[id.(float64)] = nil
+	}
+	for _, id := range actualStoreID {
+		if _, ok := count[id.(float64)]; !ok {
+			return false
+		}
+	}
+	return true
+}