scheduler: support multi priorities api for hot-region-scheduler #3899

Yisaer · 2021-07-22T05:00:39Z

Signed-off-by: yisaer disxiaofei@163.com

What problem does this PR solve?

What is changed and how it works?

Support the api for priorities for balance-hot-region-scheduler

Check List

Tests

Unit test

Release note

None

Signed-off-by: yisaer <disxiaofei@163.com>

lhy1024 · 2021-07-22T05:09:01Z

tests/pdctl/scheduler/scheduler_test.go

+	expected1["read-priorities"] = []interface{}{"key"}
+	mustExec([]string{"-u", pdAddr, "scheduler", "config", "balance-hot-region-scheduler"}, &conf1)
+	c.Assert(conf1, DeepEquals, expected1)
+	mustExec([]string{"-u", pdAddr, "scheduler", "config", "balance-hot-region-scheduler", "set", "read-priorities", "key,byte"}, nil)


What will happen to it if the string is empty or otherwise unparseable？

Signed-off-by: yisaer <disxiaofei@163.com>

codecov · 2021-07-22T05:23:18Z

Codecov Report

Merging #3899 (0133b1d) into master (9e15727) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3899      +/-   ##
==========================================
- Coverage   74.56%   74.49%   -0.08%     
==========================================
  Files         246      246              
  Lines       24868    24885      +17     
==========================================
- Hits        18543    18537       -6     
- Misses       4670     4687      +17     
- Partials     1655     1661       +6

Flag	Coverage Δ
unittests	`74.49% <100.00%> (-0.08%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/schedulers/hot_region_config.go	`83.00% <100.00%> (+0.34%)`	⬆️
tools/pd-ctl/pdctl/command/scheduler.go	`72.86% <100.00%> (+1.93%)`	⬆️
server/tso/local_allocator.go	`71.62% <0.00%> (-6.76%)`	⬇️
server/region_syncer/server.go	`83.33% <0.00%> (-6.07%)`	⬇️
server/region_syncer/client.go	`78.19% <0.00%> (-4.52%)`	⬇️
server/election/leadership.go	`83.90% <0.00%> (-3.45%)`	⬇️
server/schedule/operator/step.go	`67.44% <0.00%> (-1.67%)`	⬇️
server/tso/allocator_manager.go	`68.11% <0.00%> (-1.34%)`	⬇️
server/core/storage.go	`67.68% <0.00%> (-0.77%)`	⬇️
server/cluster/coordinator.go	`73.42% <0.00%> (-0.70%)`	⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e15727...0133b1d. Read the comment docs.

Signed-off-by: yisaer <disxiaofei@163.com>

lhy1024

the rest LGTM

lhy1024 · 2021-07-26T03:53:38Z

server/schedulers/hot_region_config.go

@@ -30,6 +30,15 @@ import (
 	"github.com/unrolled/render"
 )

+const (
+	// NonePriority indicates there is no dim priority for hot-region-scheduler
+	NonePriority = ""


I think that it might be disturbing for users to think that None means no priority, meaning that each dimension has the same priority

I think our current implement already has priority, it looks:

read-priorities [key, byte] write-priorities [btye, key]

am I right?

rleungx · 2021-07-26T05:57:02Z

tests/pdctl/scheduler/scheduler_test.go

@@ -269,6 +269,8 @@ func (s *schedulerTestSuite) TestScheduler(c *C) {
 		"minor-dec-ratio":            0.99,
 		"src-tolerance-ratio":        1.05,
 		"dst-tolerance-ratio":        1.05,
+		"read-priorities":            []interface{}{},


Perhaps, we can use a default priority.

Signed-off-by: yisaer <disxiaofei@163.com>

rleungx · 2021-07-27T08:58:41Z

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

+		priorities := make([]string, 0)
+		for _, priority := range strings.Split(value, ",") {
+			if priority != schedulers.BytePriority && priority != schedulers.KeyPriority {
+				cmd.Println(fmt.Sprintf("priority should be one of %s,%s,%s",


no need none anymore?

Signed-off-by: yisaer <disxiaofei@163.com>

rleungx · 2021-07-27T10:08:57Z

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

+		prioritiesMap := make(map[string]struct{}, 0)
+		for _, priority := range strings.Split(value, ",") {
+			if priority != schedulers.BytePriority && priority != schedulers.KeyPriority {
+				cmd.Println(fmt.Sprintf("priority should be one of %s,%s",


How about

Suggested change

cmd.Println(fmt.Sprintf("priority should be one of %s,%s",

cmd.Println(fmt.Sprintf("priority should be one of [%s, %s]",

rleungx

The rest LGTM

ti-chi-bot · 2021-07-27T10:09:16Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lhy1024
rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer · 2021-07-27T10:15:40Z

/merge

ti-chi-bot · 2021-07-27T10:15:41Z

@Yisaer: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2021-07-27T10:15:43Z

This pull request has been accepted and is ready to merge.

Commit hash: a0d9fd5

Yisaer added 2 commits July 22, 2021 12:49

support api

0cdc90c

Signed-off-by: yisaer <disxiaofei@163.com>

support api

9641180

Signed-off-by: yisaer <disxiaofei@163.com>

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 22, 2021

ti-chi-bot requested review from lhy1024 and rleungx July 22, 2021 05:00

lhy1024 reviewed Jul 22, 2021

View reviewed changes

support api

7c3b792

Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer added 2 commits July 22, 2021 13:30

address the comment

467c36e

Signed-off-by: yisaer <disxiaofei@163.com>

address the comment

fcd72d2

Signed-off-by: yisaer <disxiaofei@163.com>

lhy1024 reviewed Jul 26, 2021

View reviewed changes

rleungx reviewed Jul 26, 2021

View reviewed changes

address the comment

807a828

Signed-off-by: yisaer <disxiaofei@163.com>

Yisaer force-pushed the support_multy_priority_api branch from f4b136b to 807a828 Compare July 27, 2021 08:45

Yisaer added 2 commits July 27, 2021 16:49

address the comment

2d2c299

Signed-off-by: yisaer <disxiaofei@163.com>

address the comment

610212a

Signed-off-by: yisaer <disxiaofei@163.com>

lhy1024 approved these changes Jul 27, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 27, 2021

rleungx reviewed Jul 27, 2021

View reviewed changes

Yisaer added 2 commits July 27, 2021 17:08

address the comment

e65b232

Signed-off-by: yisaer <disxiaofei@163.com>

address the comment

b3e6bd9

Signed-off-by: yisaer <disxiaofei@163.com>

rleungx reviewed Jul 27, 2021

View reviewed changes

rleungx approved these changes Jul 27, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 27, 2021

address the comment

a0d9fd5

Signed-off-by: yisaer <disxiaofei@163.com>

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 27, 2021

Merge branch 'master' into support_multy_priority_api

0133b1d

ti-chi-bot merged commit 127df63 into tikv:master Jul 27, 2021

lhy1024 mentioned this pull request Jul 28, 2021

Adjustable Priority Multidimensional Hot Scheduler Tasklist #3869

Closed

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: support multi priorities api for hot-region-scheduler #3899

scheduler: support multi priorities api for hot-region-scheduler #3899

Yisaer commented Jul 22, 2021

lhy1024 Jul 22, 2021

Yisaer Jul 22, 2021

codecov bot commented Jul 22, 2021 •

edited

Loading

lhy1024 left a comment

lhy1024 Jul 26, 2021

nolouch Jul 26, 2021

Yisaer Jul 27, 2021

rleungx Jul 26, 2021

Yisaer Jul 27, 2021

rleungx Jul 27, 2021

rleungx Jul 27, 2021

rleungx left a comment

ti-chi-bot commented Jul 27, 2021

Yisaer commented Jul 27, 2021

ti-chi-bot commented Jul 27, 2021

ti-chi-bot commented Jul 27, 2021

	cmd.Println(fmt.Sprintf("priority should be one of %s,%s",
	cmd.Println(fmt.Sprintf("priority should be one of [%s, %s]",

scheduler: support multi priorities api for hot-region-scheduler #3899

scheduler: support multi priorities api for hot-region-scheduler #3899

Conversation

Yisaer commented Jul 22, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 22, 2021 • edited Loading

Codecov Report

lhy1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Jul 27, 2021

Yisaer commented Jul 27, 2021

ti-chi-bot commented Jul 27, 2021

ti-chi-bot commented Jul 27, 2021

codecov bot commented Jul 22, 2021 •

edited

Loading