*: instead scheduler type string of types.CheckerSchedulerType #8485

okJiang · 2024-08-02T09:04:36Z

What problem does this PR solve?

Issue Number: Ref #8379

What is changed and how does it work?

- Remove the old scheduler type, such as BalanceLeaderType
- Instead old scheduler type string of types.CheckerSchedulerType
- Instead old scheduler name of types.CheckerSchedulerType in SchedulerRegisterMap

Check List

Tests

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

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

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

ti-chi-bot · 2024-08-02T09:04:38Z

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: okJiang <[email protected]>

ti-chi-bot · 2024-08-05T04:59:10Z

PR needs rebase.

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 kubernetes/test-infra repository.

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

codecov · 2024-08-05T09:00:50Z

Codecov Report

Attention: Patch coverage is 85.29412% with 15 lines in your changes missing coverage. Please review.

Project coverage is 77.38%. Comparing base (7741924) to head (ebcc655).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8485      +/-   ##
==========================================
- Coverage   77.40%   77.38%   -0.02%     
==========================================
  Files         472      472              
  Lines       61800    61826      +26     
==========================================
+ Hits        47838    47846       +8     
- Misses      10395    10415      +20     
+ Partials     3567     3565       -2

Flag	Coverage Δ
unittests	`77.38% <85.29%> (-0.02%)`	⬇️

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

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

okJiang · 2024-08-05T10:03:54Z

pkg/schedule/coordinator.go

@@ -275,7 +276,7 @@ func (c *Coordinator) InitSchedulers(needRun bool) {
 		typ := schedulers.FindSchedulerTypeByName(name)
 		var cfg sc.SchedulerConfig
 		for _, c := range scheduleCfg.Schedulers {
-			if c.Type == typ {
+			if c.Type == types.SchedulerTypeCompatibleMap[typ] {


How about renaming to ConvertNewType2OldStr

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

rleungx · 2024-08-07T04:05:39Z

pkg/schedule/type/type.go

+	}
+
+	// SchedulerStr2Type is a map to convert the scheduler string to the CheckerSchedulerType.
+	SchedulerStr2Type = map[string]CheckerSchedulerType{


StringToSchedulerType

rleungx

The rest LGTM

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

lhy1024

the rest LGTM

lhy1024 · 2024-08-08T03:58:10Z

pkg/schedule/schedulers/balance_leader.go

@@ -43,8 +43,6 @@ import (
 const (
 	// BalanceLeaderName is balance leader scheduler name.
 	BalanceLeaderName = "balance-leader-scheduler"


Do we will unify it?

It will be in the next pr

lhy1024 · 2024-08-08T04:02:11Z

pkg/mcs/scheduling/server/cluster.go

@@ -309,11 +309,12 @@ func (c *Cluster) updateScheduler() {
 		)
 		// Create the newly added schedulers.
 		for _, scheduler := range latestSchedulersConfig {
+			schedulerType := types.ConvertOldStr2Type[scheduler.Type]


How does ConvertOldStrToType sound to you? Variable names with embedded numbers are not common in PD.

They are same for me. I can update it

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

ti-chi-bot · 2024-08-08T08:11:09Z

[LGTM Timeline notifier]

Timeline:

2024-08-07 04:08:37.757777715 +0000 UTC m=+411447.624876804: ☑️ agreed by rleungx.
2024-08-08 08:11:08.650574412 +0000 UTC m=+512398.517673586: ☑️ agreed by lhy1024.

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

ti-chi-bot · 2024-08-08T11:45:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lhy1024, niubell, 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~~ [lhy1024,rleungx]
~~pkg/schedule/config/OWNERS~~ [niubell]

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

save work

ede4112

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

okJiang added 2 commits August 5, 2024 10:32

fix static

d48eaa0

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

remove type

915e6ba

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

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 5, 2024

okJiang added 5 commits August 5, 2024 15:11

Merge branch 'master' of github.com:tikv/pd into name5

fdd8821

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

fix lint

1f20ae6

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

fix ci

3ada3f5

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

rename variable

912cde3

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

fix unit test

95fc46b

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

okJiang marked this pull request as ready for review August 5, 2024 09:01

Merge branch 'master' into name5

43a5eb1

okJiang changed the title ~~[wip]: instead of scheduler type~~ *: instead scheduler type string of new type Aug 5, 2024

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 Aug 5, 2024

remove duplicate

d227f56

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

okJiang commented Aug 5, 2024

View reviewed changes

okJiang changed the title *: instead scheduler type string of new type *: instead scheduler type string of types.CheckerSchedulerType Aug 5, 2024

okJiang added 2 commits August 6, 2024 10:50

Merge branch 'name5' of github.com:okJiang/pd into name5

87acf7f

add some comment

f91836a

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

rleungx reviewed Aug 7, 2024

View reviewed changes

rleungx approved these changes Aug 7, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 7, 2024

okJiang added 2 commits August 7, 2024 13:11

fix comment

7d38383

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

Merge branch 'master' into name5

25f5f1a

lhy1024 reviewed Aug 8, 2024

View reviewed changes

okJiang added 2 commits August 8, 2024 14:48

fix comment: rename

bdb4d19

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

Merge branch 'master' of https://github.com/tikv/pd into name5

afc4fbe

lhy1024 approved these changes Aug 8, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 8, 2024

Merge branch 'master' of https://github.com/tikv/pd into name5

f9362a0

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2024

trigger ci

ebcc655

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

niubell approved these changes Aug 8, 2024

View reviewed changes

ti-chi-bot bot added the approved label Aug 8, 2024

ti-chi-bot bot merged commit 8644ec6 into tikv:master Aug 8, 2024
25 checks passed

okJiang deleted the name5 branch August 8, 2024 13:52

okJiang mentioned this pull request Aug 12, 2024

Standardize the use of scheduler name/type and checker name/type #8379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: instead scheduler type string of types.CheckerSchedulerType #8485

*: instead scheduler type string of types.CheckerSchedulerType #8485

okJiang commented Aug 2, 2024 •

edited

Loading

ti-chi-bot bot commented Aug 2, 2024

ti-chi-bot bot commented Aug 5, 2024

codecov bot commented Aug 5, 2024 •

edited

Loading

okJiang Aug 5, 2024

rleungx Aug 7, 2024

rleungx left a comment

lhy1024 left a comment

lhy1024 Aug 8, 2024

okJiang Aug 8, 2024

lhy1024 Aug 8, 2024

okJiang Aug 8, 2024

ti-chi-bot bot commented Aug 8, 2024

ti-chi-bot bot commented Aug 8, 2024

*: instead scheduler type string of types.CheckerSchedulerType #8485

*: instead scheduler type string of types.CheckerSchedulerType #8485

Conversation

okJiang commented Aug 2, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Aug 2, 2024

ti-chi-bot bot commented Aug 5, 2024

codecov bot commented Aug 5, 2024 • edited Loading

Codecov Report

okJiang Aug 5, 2024

Choose a reason for hiding this comment

rleungx Aug 7, 2024

Choose a reason for hiding this comment

rleungx left a comment

Choose a reason for hiding this comment

lhy1024 left a comment

Choose a reason for hiding this comment

lhy1024 Aug 8, 2024

Choose a reason for hiding this comment

okJiang Aug 8, 2024

Choose a reason for hiding this comment

lhy1024 Aug 8, 2024

Choose a reason for hiding this comment

okJiang Aug 8, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Aug 8, 2024

[LGTM Timeline notifier]

ti-chi-bot bot commented Aug 8, 2024

okJiang commented Aug 2, 2024 •

edited

Loading

codecov bot commented Aug 5, 2024 •

edited

Loading