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

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]>

pkg/mcs/scheduling/server/cluster.go

pkg/schedule/operator/kind.go

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.

pkg/schedule/types/type.go

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

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.

the key range may be some partition of one table, and we can decode this key by ctl.

The readability is too poor.

understand, I will add one property to record the table name.

pkg/schedule/schedulers/balance_key_range.go

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

nolouch · 2025-01-23T09:01:09Z

pkg/schedule/schedulers/balance_range.go

+	Engine    string          `json:"engine"`
+	Timeout   time.Duration   `json:"timeout"`
+	Ranges    []core.KeyRange `json:"ranges"`
+	TableName string          `json:"table-name"`


Name it as alias for better, since pd does not understand the table concept.

rleungx

The rest LGTM except the left comment.

rleungx · 2025-01-23T09:11:33Z

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

+		Use:        "balance-range-scheduler [--format=raw|encode|hex] <engine> <role> <table-name> <start_key> <end_key>",
+		Short:      "add a scheduler to balance region for given range",
+		Run:        addSchedulerForBalanceRangeCommandFunc,
+		Deprecated: "balance-range will be deprecated in the future, please use sql instead",


It's strange to add Deprecated now.

ti-chi-bot · 2025-01-23T09:15:28Z

[LGTM Timeline notifier]

Timeline:

2025-01-23 09:15:28.807279505 +0000 UTC m=+345056.138198909: ☑️ agreed by rleungx.

ti-chi-bot · 2025-01-23T09:15:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rleungx

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rleungx]

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

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]>

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

pkg/schedule/schedulers/balance_key_range.go Outdated Show resolved Hide resolved

pkg/schedule/schedulers/balance_key_range.go Outdated Show resolved Hide resolved

pkg/schedule/schedulers/balance_key_range.go Outdated Show resolved Hide resolved

rename

0696ba6

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

bufferflies force-pushed the balance_key_range branch from eabfb27 to 0696ba6 Compare January 21, 2025 07:05

add table configuration

5d5ee0f

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

nolouch reviewed Jan 23, 2025

View reviewed changes

rleungx approved these changes Jan 23, 2025

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jan 23, 2025

ti-chi-bot bot added the approved label Jan 23, 2025

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

codecov bot commented Jan 13, 2025 •

edited

Loading

rleungx Jan 15, 2025

bufferflies Jan 15, 2025

rleungx Jan 15, 2025

bufferflies Jan 16, 2025

nolouch Jan 17, 2025 •

edited

Loading

bufferflies Jan 21, 2025

nolouch Jan 22, 2025

bufferflies Jan 22, 2025

bufferflies Jan 22, 2025

nolouch Jan 23, 2025

rleungx left a comment

rleungx Jan 23, 2025

ti-chi-bot bot commented Jan 23, 2025

ti-chi-bot bot commented Jan 23, 2025

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

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

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

Choose a reason for hiding this comment

rleungx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jan 23, 2025

[LGTM Timeline notifier]

ti-chi-bot bot commented Jan 23, 2025

codecov bot commented Jan 13, 2025 •

edited

Loading

nolouch Jan 17, 2025 •

edited

Loading