api: add a new scheduler to balance the regions of the given key range #8988

bufferflies · 2025-01-10T01:51:53Z

What problem does this PR solve?

Issue Number: Close #8987

What is changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

Code changes

Side effects

Related changes

Release note

None.

Signed-off-by: 童剑 <[email protected]>

ti-chi-bot · 2025-01-10T01:51:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ti-chi-bot · 2025-01-10T01:52:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from bufferflies, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: 童剑 <[email protected]>

codecov · 2025-01-13T09:24:54Z

Codecov Report

Attention: Patch coverage is 64.25121% with 74 lines in your changes missing coverage. Please review.

Project coverage is 76.32%. Comparing base (75cdec4) to head (8bdb7bc).
Report is 1105 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8988      +/-   ##
==========================================
+ Coverage   74.91%   76.32%   +1.40%     
==========================================
  Files         416      466      +50     
  Lines       42103    70740   +28637     
==========================================
+ Hits        31543    53994   +22451     
- Misses       7810    13391    +5581     
- Partials     2750     3355     +605

Flag	Coverage Δ
unittests	`76.32% <64.25%> (+1.40%)`	⬆️

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

Signed-off-by: 童剑 <[email protected]>

rleungx · 2025-01-15T07:15:06Z

pkg/mcs/scheduling/server/cluster.go

-			schedulerType := types.ConvertOldStrToType[scheduler.Type]
+			schedulerType, ok := types.ConvertOldStrToType[scheduler.Type]
+			if !ok {
+				log.Error("scheduler not found  ", zap.String("type", scheduler.Type))


Suggested change

log.Error("scheduler not found ", zap.String("type", scheduler.Type))

log.Error("scheduler not found", zap.String("type", scheduler.Type))

rleungx · 2025-01-15T07:15:38Z

pkg/schedule/operator/kind.go

@@ -35,6 +35,8 @@ const (
 	OpMerge
 	// Initiated by range scheduler.
 	OpRange
+	// Initiated by key range scheduler.
+	OpKeyRange


Can we just reuse range?

rleungx · 2025-01-15T07:17:51Z

pkg/schedule/schedulers/balance_key_range.go

+type balanceKeyRangeSchedulerConfig struct {
+	syncutil.RWMutex
+	schedulerConfig
+	balanceKeyRangeSchedulerParam


Shall we use a slice to support multiple key ranges with different roles or engines?

We need to support a multi-key range with same role and engine.

rleungx · 2025-01-15T07:18:59Z

pkg/schedule/types/type.go

@@ -143,6 +147,7 @@ var (
 		"split-bucket-scheduler":            SplitBucketScheduler,
 		"transfer-witness-leader-scheduler": TransferWitnessLeaderScheduler,
 		"label-scheduler":                   LabelScheduler,
+		"balance-key-range-scheduler":       BalanceKeyRangeScheduler,


How about balance range scheduler?

rleungx · 2025-01-15T07:19:39Z

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

+	})
+	re.Equal(float64(time.Hour.Nanoseconds()), conf["timeout"])
+	ranges := conf["ranges"].([]any)[0].(map[string]any)
+	re.Equal(base64.StdEncoding.EncodeToString([]byte("a")), ranges["start-key"])


good catch, fixed it

Signed-off-by: 童剑 <[email protected]>

rleungx · 2025-01-15T10:25:59Z

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

@@ -374,6 +375,17 @@ func NewBalanceWitnessSchedulerCommand() *cobra.Command {
 	return c
 }

+// NewBalanceRangeSchedulerCommand returns a command to add a balance-key-range-scheduler.
+func NewBalanceRangeSchedulerCommand() *cobra.Command {


Do we need to support it? It's hard to use.

yes, it's helpful to debug it in the first step, and then it recommends the SQL function description

Signed-off-by: 童剑 <[email protected]>

nolouch · 2025-01-17T07:09:08Z

pkg/schedule/schedulers/balance_key_range.go

+	Role    string          `json:"role"`
+	Engine  string          `json:"engine"`
+	Timeout time.Duration   `json:"timeout"`
+	Ranges  []core.KeyRange `json:"ranges"`


I wonder if we can let the API interface like:

type KeyRange strut { Alias string `json:"alias"` KeyRange string `json:"range"` }

Then we can alias the table name by tidb or others to help the user read it.

nolouch · 2025-01-17T07:11:17Z

pkg/schedule/schedulers/balance_key_range.go

+type balanceKeyRangeSchedulerParam struct {
+	Role    string          `json:"role"`
+	Engine  string          `json:"engine"`
+	Timeout time.Duration   `json:"timeout"`


Do all ranges share the same timeout?

rleungx · 2025-01-17T08:40:45Z

pkg/schedule/schedulers/balance_key_range.go

@@ -0,0 +1,151 @@
+package schedulers


Need a license and please also change the filename.

rleungx · 2025-01-17T08:42:01Z

pkg/schedule/schedulers/balance_key_range.go

+	"github.com/tikv/pd/pkg/utils/syncutil"
+)
+
+type balanceKeyRangeSchedulerHandler struct {


Keep using balanceRangeSchedulerHandler?

rleungx · 2025-01-17T08:44:09Z

pkg/schedule/schedulers/balance_key_range.go

+}
+
+// EncodeConfig serializes the config.
+func (s *balanceKeyRangeScheduler) EncodeConfig() ([]byte, error) {


It's a little bit confusing to have both EncodeConfig and encodeConfig.

bufferflies added 2 commits January 7, 2025 18:24

add scheduler config

e693b3e

Signed-off-by: 童剑 <[email protected]>

add new scheduler for key range

23ff7d0

Signed-off-by: 童剑 <[email protected]>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 10, 2025

ti-chi-bot bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 10, 2025

bufferflies added 2 commits January 10, 2025 14:32

pass ut

1e6d628

Signed-off-by: 童剑 <[email protected]>

lint

d1da5b5

Signed-off-by: 童剑 <[email protected]>

bufferflies force-pushed the balance_key_range branch 2 times, most recently from fe71a54 to cb8a4b9 Compare January 14, 2025 08:21

pass ut

d0cfc2d

Signed-off-by: 童剑 <[email protected]>

bufferflies force-pushed the balance_key_range branch from cb8a4b9 to d0cfc2d Compare January 14, 2025 09:32

bufferflies marked this pull request as ready for review January 14, 2025 09:56

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2025

rleungx reviewed Jan 15, 2025

View reviewed changes

rename balance-key-range to balance-range

d86148f

Signed-off-by: 童剑 <[email protected]>

rleungx reviewed Jan 15, 2025

View reviewed changes

use hex encode

8bdb7bc

Signed-off-by: 童剑 <[email protected]>

bufferflies force-pushed the balance_key_range branch from 0bb25b9 to 8bdb7bc Compare January 16, 2025 07:36

bufferflies changed the title ~~api: add new scheduler for balance the regions of the given key range~~ api: add a new scheduler to balance the regions of the given key range Jan 17, 2025

nolouch reviewed Jan 17, 2025

View reviewed changes

rleungx reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: add a new scheduler to balance the regions of the given key range #8988

api: add a new scheduler to balance the regions of the given key range #8988

bufferflies commented Jan 10, 2025

ti-chi-bot bot commented Jan 10, 2025

ti-chi-bot bot commented Jan 10, 2025

codecov bot commented Jan 13, 2025 •

edited

Loading

rleungx Jan 15, 2025

bufferflies Jan 16, 2025

rleungx Jan 15, 2025

bufferflies Jan 16, 2025

rleungx Jan 15, 2025

bufferflies Jan 15, 2025

rleungx Jan 15, 2025

bufferflies Jan 16, 2025

rleungx Jan 15, 2025

bufferflies Jan 16, 2025

rleungx Jan 15, 2025

bufferflies Jan 16, 2025

nolouch Jan 17, 2025 •

edited

Loading

nolouch Jan 17, 2025

rleungx Jan 17, 2025

rleungx Jan 17, 2025

rleungx Jan 17, 2025

	log.Error("scheduler not found ", zap.String("type", scheduler.Type))
	log.Error("scheduler not found", zap.String("type", scheduler.Type))

api: add a new scheduler to balance the regions of the given key range #8988

Are you sure you want to change the base?

api: add a new scheduler to balance the regions of the given key range #8988

Conversation

bufferflies commented Jan 10, 2025

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Jan 10, 2025

ti-chi-bot bot commented Jan 10, 2025

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

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

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

nolouch Jan 17, 2025 • edited Loading

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

codecov bot commented Jan 13, 2025 •

edited

Loading

nolouch Jan 17, 2025 •

edited

Loading