*: decouple the dependency between config and schedule #5792

rleungx · 2022-12-16T10:45:31Z

What problem does this PR solve?

Issue Number: Ref #5838.

What is changed and how does it work?

Check List

Tests

Unit test

Release note

None

ti-chi-bot · 2022-12-16T10:45:33Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

HunDunDM
nolouch

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.

codecov · 2022-12-16T10:53:39Z

Codecov Report

Patch coverage: 87.61% and project coverage change: -0.12 ⚠️

Comparison is base (6fa2bc9) 74.10% compared to head (988bdcb) 73.99%.

❗ Current head 988bdcb differs from pull request most recent head 5c17fd4. Consider uploading reports for the commit 5c17fd4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5792      +/-   ##
==========================================
- Coverage   74.10%   73.99%   -0.12%     
==========================================
  Files         381      383       +2     
  Lines       37809    37808       -1     
==========================================
- Hits        28020    27975      -45     
- Misses       7335     7365      +30     
- Partials     2454     2468      +14

Flag	Coverage Δ
unittests	`73.99% <87.61%> (-0.12%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/mock/mockcluster/mockcluster.go	`94.25% <ø> (ø)`
server/api/scheduler.go	`40.48% <ø> (ø)`
server/cluster/diagnostic_manager.go	`76.34% <ø> (ø)`
server/config/config.go	`75.92% <ø> (-0.11%)`	⬇️
server/config/util.go	`82.60% <ø> (-4.06%)`	⬇️
server/handler.go	`52.78% <ø> (ø)`
server/schedule/filter/healthy.go	`90.00% <ø> (ø)`
server/schedule/operator/builder.go	`86.27% <ø> (ø)`
server/schedule/scheduler.go	`55.26% <ø> (ø)`
server/schedule/schedulers/balance_plan.go	`86.48% <ø> (ø)`
... and 73 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

nolouch · 2022-12-19T16:43:39Z

server/schedule/config/config.go

+}
+
+// Config is the interface that wraps the Config related method.
+type Config interface {


Each scheduler also has it's own config, How to distinguish them?

A config instance that implements these interfaces can be used to start the scheduling services. And this interface is used for compatibility. As for the scheduler config, it doesn't rely on the server package so it can be defined without implementing this interface.

maybe we can reduce the functions.

nolouch

lgtm

nolouch · 2022-12-26T05:24:00Z

server/schedule/config/config.go

+}
+
+// Config is the interface that wraps the Config related method.
+type Config interface {


maybe we can reduce the functions.

lhy1024 · 2022-12-27T10:57:06Z

server/config/config.go

@@ -995,11 +995,6 @@ func (c *ScheduleConfig) Validate() error {
 	if c.LeaderSchedulePolicy != "count" && c.LeaderSchedulePolicy != "size" {
 		return errors.Errorf("leader-schedule-policy %v is invalid", c.LeaderSchedulePolicy)
 	}
-	for _, scheduleConfig := range c.Schedulers {


Is it removed?

server/schedule/config/config.go

rleungx · 2023-01-03T03:37:12Z

@binshi-bing

binshi-bing · 2023-01-25T17:34:23Z

pkg/mock/mockcluster/mockcluster.go

@@ -32,6 +32,7 @@ import (
 	"github.com/tikv/pd/server/config"
 	"github.com/tikv/pd/server/core"
 	"github.com/tikv/pd/server/core/storelimit"
+	sc "github.com/tikv/pd/server/schedule/config"


Do we favor short name over a full name? 'sc' could be read as store config? "scheduleConfig" seems to be a clearer name to me.

binshi-bing · 2023-01-25T17:49:31Z

server/config/store_config.go

@@ -219,6 +233,14 @@ func (m *StoreConfigManager) GetStoreConfig() *StoreConfig {
 	return config.(*StoreConfig)
 }

+// SetStoreConfig sets the store configuration.
+func (m *StoreConfigManager) SetStoreConfig(cfg *StoreConfig) {
+	if m == nil {


if m != nil {
m.config.Store(cfg)
}

binshi-bing · 2023-01-25T17:55:44Z

server/config/store_config.go

@@ -120,6 +120,14 @@ func (c *StoreConfig) IsEnableRegionBucket() bool {
 	return c.Coprocessor.EnableRegionBucket
 }

+// SetRegionBucketEnabled sets if the region bucket is enabled.
+func (c *StoreConfig) SetRegionBucketEnabled(enabled bool) {
+	if c == nil {


if c != nil {
c.Coprocessor.EnableRegionBucket = enabled
}

binshi-bing · 2023-01-25T18:03:21Z

server/schedule/config/config.go

+var schedulerMap sync.Map
+
+// RegisterScheduler registers the scheduler type.
+func RegisterScheduler(typ string) {


'type' is a reserved keyword, but I'm wondering if we can use a better name than 'typ'? how about schedulerType?

binshi-bing · 2023-01-25T21:07:48Z

server/schedule/config/config.go

+}
+
+// Config is the interface that wraps the Config related methods.
+type Config interface {


I feel this part is hard to maintain and is error-prone in the future. Just wondering if we can reuse ScheduleConfig in server/config/config.go and move it to server/scheduler to remove dependency. It seems that we want to remove the dependency (on server/config) from server/scheduler, but I'm sure if we can let server/config depend on server/scheduler/config. I mightn't fully understand our work here. I'll sync with you two after holiday. @rleungx @nolouch ^

Signed-off-by: Ryan Leung <rleungx@gmail.com>

binshi-bing

LGTM

ti-chi-bot · 2023-03-02T22:57:22Z

@binshi-bing: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM

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.

rleungx · 2023-03-03T03:31:03Z

/run-all-tests

rleungx · 2023-03-03T05:35:44Z

/merge

ti-chi-bot · 2023-03-03T05:35:45Z

@rleungx: 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 · 2023-03-03T05:35:47Z

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

Commit hash: 4509a2c

ti-chi-bot · 2023-03-03T05:37:20Z

@rleungx: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. labels Dec 16, 2022

ti-chi-bot requested review from HunDunDM and nolouch December 16, 2022 10:46

ti-chi-bot removed the do-not-merge/needs-linked-issue label Dec 19, 2022

nolouch reviewed Dec 19, 2022

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2022

nolouch approved these changes Dec 26, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 26, 2022

lhy1024 reviewed Dec 27, 2022

View reviewed changes

rleungx force-pushed the schedule-config branch from 91fa22c to 91e9dcf Compare December 30, 2022 01:48

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 30, 2022

rleungx mentioned this pull request Jan 9, 2023

Decouple the dependency between different components #5838

Closed

10 tasks

binshi-bing reviewed Jan 25, 2023

View reviewed changes

rleungx added 3 commits March 2, 2023 11:57

decouple the logic between config and schedule

df1e9c2

Signed-off-by: Ryan Leung <rleungx@gmail.com>

move register scheduler to schedule

ca7a363

Signed-off-by: Ryan Leung <rleungx@gmail.com>

typo

82c35a1

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the schedule-config branch from 51b6bed to 988bdcb Compare March 2, 2023 05:39

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

resolve conflicts

86532c2

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the schedule-config branch from 988bdcb to 86532c2 Compare March 2, 2023 08:15

binshi-bing approved these changes Mar 2, 2023

View reviewed changes

HunDunDM approved these changes Mar 2, 2023

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 Mar 2, 2023

HunDunDM mentioned this pull request Mar 3, 2023

Separate scheduling into an independent service #5839

Closed

Merge branch 'master' into schedule-config

a8fef6d

Merge branch 'master' into schedule-config

4509a2c

rleungx closed this Mar 3, 2023

rleungx reopened this Mar 3, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 3, 2023

Merge branch 'master' into schedule-config

5c17fd4

ti-chi-bot merged commit 05228ba into tikv:master Mar 3, 2023

rleungx deleted the schedule-config branch March 3, 2023 06:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: decouple the dependency between config and schedule #5792

*: decouple the dependency between config and schedule #5792

rleungx commented Dec 16, 2022 •

edited

Loading

ti-chi-bot commented Dec 16, 2022 •

edited

Loading

codecov bot commented Dec 16, 2022 •

edited

Loading

nolouch Dec 19, 2022

rleungx Dec 20, 2022

nolouch Dec 26, 2022

nolouch left a comment

nolouch Dec 26, 2022

lhy1024 Dec 27, 2022

rleungx Dec 30, 2022

rleungx commented Jan 3, 2023

binshi-bing Jan 25, 2023

binshi-bing Jan 25, 2023

binshi-bing Jan 25, 2023

binshi-bing Jan 25, 2023

binshi-bing Jan 25, 2023

binshi-bing left a comment

ti-chi-bot commented Mar 2, 2023

rleungx commented Mar 3, 2023

rleungx commented Mar 3, 2023

ti-chi-bot commented Mar 3, 2023

ti-chi-bot commented Mar 3, 2023

ti-chi-bot commented Mar 3, 2023

*: decouple the dependency between config and schedule #5792

*: decouple the dependency between config and schedule #5792

Conversation

rleungx commented Dec 16, 2022 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Dec 16, 2022 • edited Loading

codecov bot commented Dec 16, 2022 • 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

nolouch 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

rleungx commented Jan 3, 2023

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

binshi-bing left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Mar 2, 2023

rleungx commented Mar 3, 2023

rleungx commented Mar 3, 2023

ti-chi-bot commented Mar 3, 2023

ti-chi-bot commented Mar 3, 2023

ti-chi-bot commented Mar 3, 2023

rleungx commented Dec 16, 2022 •

edited

Loading

ti-chi-bot commented Dec 16, 2022 •

edited

Loading

codecov bot commented Dec 16, 2022 •

edited

Loading