Repository: tikv/pd (go)

Issue:
No update logs when set balance-hot-region-scheduler attributes
## Bug Report

<!-- Please answer these questions before submitting your issue. Thanks! -->

### What did you do?
/var/lib/pd/log # grep 'config is updated' * |awk -F" " '{print $1,$2,$3,$4,$5,$6,$7,$8,$9}'
[2022/01/20 10:01:37.910 +00:00] [INFO] [server.go:957] ["PD server config is
[2022/01/20 11:10:56.768 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/20 11:12:36.790 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/20 11:14:16.774 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/20 11:15:56.775 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/20 11:17:36.783 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/20 11:19:16.775 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/20 11:20:56.783 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/20 11:21:25.572 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/21 05:44:51.526 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/21 05:46:31.542 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/21 05:48:11.541 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/21 05:49:51.532 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/21 05:51:31.540 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/21 05:53:11.541 +00:00] [INFO] [server.go:835] ["schedule config is updated"]
[2022/01/21 05:54:05.620 +00:00] [INFO] [server.go:835] ["schedule config is updated"]

2022-01-21T13:59:06.490+0800    INFO    pd-basic/pdbasic_schedule_test.go:299   /pd-ctl scheduler config balance-hot-region-scheduler set strict-picking-store false:Success!
2022-01-21T13:59:06.490+0800    INFO    oltppkg/util.go:62      workload run    {"wait": "5m0s"}
<!-- If possible, provide a recipe for reproducing the error. -->

### What did you expect to see?
See update logs when set balance-hot-region-scheduler parameters.

### What did you see instead?
No related logs print.

### What version of PD are you using (`pd-server -V`)?
/ # /pd-server -V
Release Version: v5.4.0-nightly
Edition: Community
Git Commit Hash: 964569d45ac0685110d12665e6ade8680042c8e5
Git Branch: heads/refs/tags/v5.4.0-nightly
UTC Build Time:  2022-01-20 06:11:07

Minio logs can be get from tcms via plan id: 600774, testbed: pd-schedule-hongm-tps-600774-1-763


Pull request description:
scheduler: enhance output when scheduler config update
<!--

Thank you for working on PD! Please read PD's [CONTRIBUTING](https://github.com/tikv/pd/blob/master/CONTRIBUTING.md) document **BEFORE** filing this PR.

PR Title Format:
1. pkg [, pkg2, pkg3]: what's changed
2. *: what's changed

-->

### What problem does this PR solve?

<!--

Please create an issue first to describe the problem.
There MUST be one line starting with "Issue Number:  " and 
linking the relevant issues via the "close" or "ref".
For more info, check https://github.com/tikv/pd/blob/master/CONTRIBUTING.md#linking-issues.

-->
Issue Number: Close #4607

### What is changed and how does it work?

<!--

You could use the "commit message" code block to add more description to the final commit message.
For more info, check https://github.com/tikv/pd/blob/master/CONTRIBUTING.md#format-of-the-commit-message.

-->

```commit-message
```
1. reduce unnecessary config check in pd-ctl.
2. return result when run successfully
3. add log when config changed

### Check List

<!-- Remove the items that are not applicable. -->

Tests

<!-- At least one of these tests must be included. -->

- Unit test
- Integration test
- Manual test (add detailed scripts or steps below)
![image](https://github.com/tikv/pd/assets/19542290/5229749f-fd41-492c-8ea0-94289f2b7269)

### Release note

<!--

A bugfix or a new feature needs a release note. If there is no need to give a release note, just leave it with the `None`.

Please refer to [Release Notes Language Style Guide](https://pingcap.github.io/tidb-dev-guide/contribute-to-tidb/release-notes-style-guide.html) to write a quality release note.

-->

```release-note
None.
```


Patch:
diff --git a/pkg/schedule/schedulers/balance_leader.go b/pkg/schedule/schedulers/balance_leader.go
index ce4bfd069..70f1cf5cb 100644
--- a/pkg/schedule/schedulers/balance_leader.go
+++ b/pkg/schedule/schedulers/balance_leader.go
@@ -90,7 +90,8 @@ func (conf *balanceLeaderSchedulerConfig) Update(data []byte) (int, interface{})
 			return http.StatusBadRequest, "invalid batch size which should be an integer between 1 and 10"
 		}
 		conf.persistLocked()
-		return http.StatusOK, "success"
+		log.Info("balance-leader-scheduler config is updated", zap.ByteString("old", oldc), zap.ByteString("new", newc))
+		return http.StatusOK, "Config is updated."
 	}
 	m := make(map[string]interface{})
 	if err := json.Unmarshal(data, &m); err != nil {
@@ -98,9 +99,9 @@ func (conf *balanceLeaderSchedulerConfig) Update(data []byte) (int, interface{})
 	}
 	ok := reflectutil.FindSameFieldByJSON(conf, m)
 	if ok {
-		return http.StatusOK, "no changed"
+		return http.StatusOK, "Config is the same with origin, so do nothing."
 	}
-	return http.StatusBadRequest, "config item not found"
+	return http.StatusBadRequest, "Config item is not found."
 }
 
 func (conf *balanceLeaderSchedulerConfig) validate() bool {
diff --git a/pkg/schedule/schedulers/balance_witness.go b/pkg/schedule/schedulers/balance_witness.go
index 556743563..b60c6d81f 100644
--- a/pkg/schedule/schedulers/balance_witness.go
+++ b/pkg/schedule/schedulers/balance_witness.go
@@ -76,7 +76,8 @@ func (conf *balanceWitnessSchedulerConfig) Update(data []byte) (int, interface{}
 			return http.StatusBadRequest, "invalid batch size which should be an integer between 1 and 10"
 		}
 		conf.persistLocked()
-		return http.StatusOK, "success"
+		log.Info("balance-witness-scheduler config is updated", zap.ByteString("old", oldc), zap.ByteString("new", newc))
+		return http.StatusOK, "Config is updated."
 	}
 	m := make(map[string]interface{})
 	if err := json.Unmarshal(data, &m); err != nil {
@@ -84,9 +85,9 @@ func (conf *balanceWitnessSchedulerConfig) Update(data []byte) (int, interface{}
 	}
 	ok := reflectutil.FindSameFieldByJSON(conf, m)
 	if ok {
-		return http.StatusOK, "no changed"
+		return http.StatusOK, "Config is the same with origin, so do nothing."
 	}
-	return http.StatusBadRequest, "config item not found"
+	return http.StatusBadRequest, "Config item is not found."
 }
 
 func (conf *balanceWitnessSchedulerConfig) validate() bool {
diff --git a/pkg/schedule/schedulers/hot_region_config.go b/pkg/schedule/schedulers/hot_region_config.go
index ae7f05c56..e0fd47c64 100644
--- a/pkg/schedule/schedulers/hot_region_config.go
+++ b/pkg/schedule/schedulers/hot_region_config.go
@@ -421,7 +421,9 @@ func (conf *hotRegionSchedulerConfig) handleSetConfig(w http.ResponseWriter, r *
 	newc, _ := json.Marshal(conf)
 	if !bytes.Equal(oldc, newc) {
 		conf.persistLocked()
-		rd.Text(w, http.StatusOK, "success")
+		log.Info("hot-region-scheduler config is updated", zap.String("old", string(oldc)), zap.String("new", string(newc)))
+		rd.Text(w, http.StatusOK, "Config is updated.")
+		return
 	}
 
 	m := make(map[string]interface{})
@@ -431,11 +433,11 @@ func (conf *hotRegionSchedulerConfig) handleSetConfig(w http.ResponseWriter, r *
 	}
 	ok := reflectutil.FindSameFieldByJSON(conf, m)
 	if ok {
-		rd.Text(w, http.StatusOK, "no changed")
+		rd.Text(w, http.StatusOK, "Config is the same with origin, so do nothing.")
 		return
 	}
 
-	rd.Text(w, http.StatusBadRequest, "config item not found")
+	rd.Text(w, http.StatusBadRequest, "Config item is not found.")
 }
 
 func (conf *hotRegionSchedulerConfig) persistLocked() error {
diff --git a/pkg/schedule/schedulers/shuffle_region_config.go b/pkg/schedule/schedulers/shuffle_region_config.go
index 97ccab136..b16f20af8 100644
--- a/pkg/schedule/schedulers/shuffle_region_config.go
+++ b/pkg/schedule/schedulers/shuffle_region_config.go
@@ -101,7 +101,7 @@ func (conf *shuffleRegionSchedulerConfig) handleSetRoles(w http.ResponseWriter,
 		rd.Text(w, http.StatusInternalServerError, err.Error())
 		return
 	}
-	rd.Text(w, http.StatusOK, "")
+	rd.Text(w, http.StatusOK, "Config is updated.")
 }
 
 func (conf *shuffleRegionSchedulerConfig) persist() error {
diff --git a/pkg/schedule/schedulers/split_bucket.go b/pkg/schedule/schedulers/split_bucket.go
index 415e62077..b5dbec7ec 100644
--- a/pkg/schedule/schedulers/split_bucket.go
+++ b/pkg/schedule/schedulers/split_bucket.go
@@ -28,6 +28,7 @@ import (
 	"github.com/tikv/pd/pkg/schedule/plan"
 	"github.com/tikv/pd/pkg/statistics/buckets"
 	"github.com/tikv/pd/pkg/storage/endpoint"
+	"github.com/tikv/pd/pkg/utils/reflectutil"
 	"github.com/tikv/pd/pkg/utils/syncutil"
 	"github.com/unrolled/render"
 )
@@ -121,10 +122,22 @@ func (h *splitBucketHandler) UpdateConfig(w http.ResponseWriter, r *http.Request
 	newc, _ := json.Marshal(h.conf)
 	if !bytes.Equal(oldc, newc) {
 		h.conf.persistLocked()
-		rd.Text(w, http.StatusOK, "success")
+		rd.Text(w, http.StatusOK, "Config is updated.")
+		return
+	}
+
+	m := make(map[string]interface{})
+	if err := json.Unmarshal(data, &m); err != nil {
+		rd.JSON(w, http.StatusInternalServerError, err.Error())
+		return
+	}
+	ok := reflectutil.FindSameFieldByJSON(h.conf, m)
+	if ok {
+		rd.Text(w, http.StatusOK, "Config is the same with origin, so do nothing.")
+		return
 	}
 
-	rd.Text(w, http.StatusBadRequest, "config item not found")
+	rd.Text(w, http.StatusBadRequest, "Config item is not found.")
 }
 
 func newSplitBucketHandler(conf *splitBucketSchedulerConfig) http.Handler {
diff --git a/server/api/scheduler.go b/server/api/scheduler.go
index cf1c82c65..dc7f25071 100644
--- a/server/api/scheduler.go
+++ b/server/api/scheduler.go
@@ -15,8 +15,8 @@
 package api
 
 import (
-	"fmt"
 	"net/http"
+	"net/url"
 	"strings"
 	"time"
 
@@ -29,8 +29,6 @@ import (
 	"github.com/unrolled/render"
 )
 
-const schedulerConfigPrefix = "pd/api/v1/scheduler-config"
-
 type schedulerHandler struct {
 	*server.Handler
 	svr *server.Server
@@ -321,8 +319,12 @@ func (h *schedulerHandler) handleErr(w http.ResponseWriter, err error) {
 func (h *schedulerHandler) redirectSchedulerDelete(w http.ResponseWriter, name, schedulerName string) {
 	args := strings.Split(name, "-")
 	args = args[len(args)-1:]
-	url := fmt.Sprintf("%s/%s/%s/delete/%s", h.GetAddr(), schedulerConfigPrefix, schedulerName, args[0])
-	statusCode, err := apiutil.DoDelete(h.svr.GetHTTPClient(), url)
+	deleteURL, err := url.JoinPath(h.GetAddr(), "pd", server.SchedulerConfigHandlerPath, schedulerName, "delete", args[0])
+	if err != nil {
+		h.r.JSON(w, http.StatusInternalServerError, err.Error())
+		return
+	}
+	statusCode, err := apiutil.DoDelete(h.svr.GetHTTPClient(), deleteURL)
 	if err != nil {
 		h.r.JSON(w, statusCode, err.Error())
 		return
diff --git a/server/handler.go b/server/handler.go
index d86b889b6..02ec6da48 100644
--- a/server/handler.go
+++ b/server/handler.go
@@ -18,8 +18,8 @@ import (
 	"bytes"
 	"encoding/hex"
 	"encoding/json"
-	"fmt"
 	"net/http"
+	"net/url"
 	"path"
 	"strconv"
 	"strings"
@@ -76,8 +76,6 @@ var (
 	ErrPluginNotFound = func(pluginPath string) error {
 		return errors.Errorf("plugin is not found: %s", pluginPath)
 	}
-
-	schedulerConfigPrefix = "pd/api/v1/scheduler-config"
 )
 
 // Handler is a helper to export methods to handle API/RPC requests.
@@ -1096,7 +1094,10 @@ func (h *Handler) redirectSchedulerUpdate(name string, storeID float64) error {
 	input := make(map[string]interface{})
 	input["name"] = name
 	input["store_id"] = storeID
-	updateURL := fmt.Sprintf("%s/%s/%s/config", h.GetAddr(), schedulerConfigPrefix, name)
+	updateURL, err := url.JoinPath(h.GetAddr(), "pd", SchedulerConfigHandlerPath, name, "config")
+	if err != nil {
+		return err
+	}
 	body, err := json.Marshal(input)
 	if err != nil {
 		return err
diff --git a/tools/pd-ctl/pdctl/command/global.go b/tools/pd-ctl/pdctl/command/global.go
index 85fe63ac8..5d8552da5 100644
--- a/tools/pd-ctl/pdctl/command/global.go
+++ b/tools/pd-ctl/pdctl/command/global.go
@@ -173,8 +173,8 @@ func requestJSON(cmd *cobra.Command, method, prefix string, input map[string]int
 	}
 
 	endpoints := getEndpoints(cmd)
+	var msg []byte
 	err = tryURLs(cmd, endpoints, func(endpoint string) error {
-		var msg []byte
 		var req *http.Request
 		var resp *http.Response
 		url := endpoint + "/" + prefix
@@ -194,11 +194,11 @@ func requestJSON(cmd *cobra.Command, method, prefix string, input map[string]int
 			return err
 		}
 		defer resp.Body.Close()
+		msg, err = io.ReadAll(resp.Body)
+		if err != nil {
+			return err
+		}
 		if resp.StatusCode != http.StatusOK {
-			msg, err = io.ReadAll(resp.Body)
-			if err != nil {
-				return err
-			}
 			return errors.Errorf("[%d] %s", resp.StatusCode, msg)
 		}
 		return nil
@@ -207,7 +207,7 @@ func requestJSON(cmd *cobra.Command, method, prefix string, input map[string]int
 		cmd.Printf("Failed! %s\n", err)
 		return
 	}
-	cmd.Println("Success!")
+	cmd.Printf("Success! %s\n", strings.Trim(string(msg), "\""))
 }
 
 func postJSON(cmd *cobra.Command, prefix string, input map[string]interface{}) {
diff --git a/tools/pd-ctl/pdctl/command/scheduler.go b/tools/pd-ctl/pdctl/command/scheduler.go
index 576580228..da37b4592 100644
--- a/tools/pd-ctl/pdctl/command/scheduler.go
+++ b/tools/pd-ctl/pdctl/command/scheduler.go
@@ -26,7 +26,6 @@ import (
 
 	"github.com/pingcap/errors"
 	"github.com/spf13/cobra"
-	"github.com/tikv/pd/pkg/statistics/utils"
 )
 
 var (
@@ -714,32 +713,7 @@ func postSchedulerConfigCommandFunc(cmd *cobra.Command, schedulerName string, ar
 		val = value
 	}
 	if schedulerName == "balance-hot-region-scheduler" && (key == "read-priorities" || key == "write-leader-priorities" || key == "write-peer-priorities") {
-		priorities := make([]string, 0)
-		prioritiesMap := make(map[string]struct{})
-		for _, priority := range strings.Split(value, ",") {
-			if priority != utils.BytePriority && priority != utils.KeyPriority && priority != utils.QueryPriority {
-				cmd.Println(fmt.Sprintf("priority should be one of [%s, %s, %s]",
-					utils.BytePriority,
-					utils.QueryPriority,
-					utils.KeyPriority))
-				return
-			}
-			if priority == utils.QueryPriority && key == "write-peer-priorities" {
-				cmd.Println("query is not allowed to be set in priorities for write-peer-priorities")
-				return
-			}
-			priorities = append(priorities, priority)
-			prioritiesMap[priority] = struct{}{}
-		}
-		if len(priorities) < 2 {
-			cmd.Println("priorities should have at least 2 dimensions")
-			return
-		}
-		input[key] = priorities
-		if len(priorities) != len(prioritiesMap) {
-			cmd.Println("priorities shouldn't be repeated")
-			return
-		}
+		input[key] = strings.Split(value, ",")
 	} else {
 		input[key] = val
 	}


Test patch:
diff --git a/server/api/scheduler_test.go b/server/api/scheduler_test.go
index 429026ec5..613de8e44 100644
--- a/server/api/scheduler_test.go
+++ b/server/api/scheduler_test.go
@@ -147,7 +147,7 @@ func (suite *scheduleTestSuite) TestAPI() {
 				// update again
 				err = tu.CheckPostJSON(testDialClient, updateURL, body,
 					tu.StatusOK(re),
-					tu.StringEqual(re, "\"no changed\"\n"))
+					tu.StringEqual(re, "\"Config is the same with origin, so do nothing.\"\n"))
 				suite.NoError(err)
 				// update invalidate batch
 				dataMap = map[string]interface{}{}
@@ -173,7 +173,7 @@ func (suite *scheduleTestSuite) TestAPI() {
 				suite.NoError(err)
 				err = tu.CheckPostJSON(testDialClient, updateURL, body,
 					tu.Status(re, http.StatusBadRequest),
-					tu.StringEqual(re, "\"config item not found\"\n"))
+					tu.StringEqual(re, "\"Config item is not found.\"\n"))
 				suite.NoError(err)
 			},
 		},
@@ -184,18 +184,29 @@ func (suite *scheduleTestSuite) TestAPI() {
 				resp := make(map[string]interface{})
 				listURL := fmt.Sprintf("%s%s%s/%s/list", suite.svr.GetAddr(), apiPrefix, server.SchedulerConfigHandlerPath, name)
 				suite.NoError(tu.ReadGetJSON(re, testDialClient, listURL, &resp))
-				expectMap := map[string]float64{
-					"min-hot-byte-rate":          100,
-					"min-hot-key-rate":           10,
-					"max-zombie-rounds":          3,
-					"max-peer-number":            1000,
+				expectMap := map[string]interface{}{
+					"min-hot-byte-rate":          100.0,
+					"min-hot-key-rate":           10.0,
+					"min-hot-query-rate":         10.0,
+					"max-zombie-rounds":          3.0,
+					"max-peer-number":            1000.0,
 					"byte-rate-rank-step-ratio":  0.05,
 					"key-rate-rank-step-ratio":   0.05,
 					"query-rate-rank-step-ratio": 0.05,
 					"count-rank-step-ratio":      0.01,
 					"great-dec-ratio":            0.95,
 					"minor-dec-ratio":            0.99,
+					"src-tolerance-ratio":        1.05,
+					"dst-tolerance-ratio":        1.05,
+					"split-thresholds":           0.2,
+					"rank-formula-version":       "v2",
+					"read-priorities":            []interface{}{"byte", "key"},
+					"write-leader-priorities":    []interface{}{"key", "byte"},
+					"write-peer-priorities":      []interface{}{"byte", "key"},
+					"enable-for-tiflash":         "true",
+					"strict-picking-store":       "true",
 				}
+				re.Equal(len(expectMap), len(resp), "expect %v, got %v", expectMap, resp)
 				for key := range expectMap {
 					suite.Equal(expectMap[key], resp[key])
 				}
@@ -209,12 +220,60 @@ func (suite *scheduleTestSuite) TestAPI() {
 				resp = make(map[string]interface{})
 				suite.NoError(tu.ReadGetJSON(re, testDialClient, listURL, &resp))
 				for key := range expectMap {
-					suite.Equal(expectMap[key], resp[key])
+					suite.Equal(expectMap[key], resp[key], "key %s", key)
 				}
 				// update again
 				err = tu.CheckPostJSON(testDialClient, updateURL, body,
 					tu.StatusOK(re),
-					tu.StringEqual(re, "no changed"))
+					tu.StringEqual(re, "Config is the same with origin, so do nothing."))
+				suite.NoError(err)
+				// config item not found
+				dataMap = map[string]interface{}{}
+				dataMap["error"] = 3
+				body, err = json.Marshal(dataMap)
+				suite.NoError(err)
+				err = tu.CheckPostJSON(testDialClient, updateURL, body,
+					tu.Status(re, http.StatusBadRequest),
+					tu.StringEqual(re, "Config item is not found."))
+				suite.NoError(err)
+			},
+		},
+		{
+			name:        "split-bucket-scheduler",
+			createdName: "split-bucket-scheduler",
+			extraTestFunc: func(name string) {
+				resp := make(map[string]interface{})
+				listURL := fmt.Sprintf("%s%s%s/%s/list", suite.svr.GetAddr(), apiPrefix, server.SchedulerConfigHandlerPath, name)
+				suite.NoError(tu.ReadGetJSON(re, testDialClient, listURL, &resp))
+				suite.Equal(3.0, resp["degree"])
+				suite.Equal(0.0, resp["split-limit"])
+				dataMap := make(map[string]interface{})
+				dataMap["degree"] = 4
+				updateURL := fmt.Sprintf("%s%s%s/%s/config", suite.svr.GetAddr(), apiPrefix, server.SchedulerConfigHandlerPath, name)
+				body, err := json.Marshal(dataMap)
+				suite.NoError(err)
+				suite.NoError(tu.CheckPostJSON(testDialClient, updateURL, body, tu.StatusOK(re)))
+				resp = make(map[string]interface{})
+				suite.NoError(tu.ReadGetJSON(re, testDialClient, listURL, &resp))
+				suite.Equal(4.0, resp["degree"])
+				// update again
+				err = tu.CheckPostJSON(testDialClient, updateURL, body,
+					tu.StatusOK(re),
+					tu.StringEqual(re, "Config is the same with origin, so do nothing."))
+				suite.NoError(err)
+				// empty body
+				err = tu.CheckPostJSON(testDialClient, updateURL, nil,
+					tu.Status(re, http.StatusInternalServerError),
+					tu.StringEqual(re, "\"unexpected end of JSON input\"\n"))
+				suite.NoError(err)
+				// config item not found
+				dataMap = map[string]interface{}{}
+				dataMap["error"] = 3
+				body, err = json.Marshal(dataMap)
+				suite.NoError(err)
+				err = tu.CheckPostJSON(testDialClient, updateURL, body,
+					tu.Status(re, http.StatusBadRequest),
+					tu.StringEqual(re, "Config item is not found."))
 				suite.NoError(err)
 			},
 		},
@@ -254,7 +313,7 @@ func (suite *scheduleTestSuite) TestAPI() {
 				// update again
 				err = tu.CheckPostJSON(testDialClient, updateURL, body,
 					tu.StatusOK(re),
-					tu.StringEqual(re, "\"no changed\"\n"))
+					tu.StringEqual(re, "\"Config is the same with origin, so do nothing.\"\n"))
 				suite.NoError(err)
 				// update invalidate batch
 				dataMap = map[string]interface{}{}
@@ -280,7 +339,7 @@ func (suite *scheduleTestSuite) TestAPI() {
 				suite.NoError(err)
 				err = tu.CheckPostJSON(testDialClient, updateURL, body,
 					tu.Status(re, http.StatusBadRequest),
-					tu.StringEqual(re, "\"config item not found\"\n"))
+					tu.StringEqual(re, "\"Config item is not found.\"\n"))
 				suite.NoError(err)
 			},
 		},