scheduler: add disable to independent config #8567
Conversation
Signed-off-by: okJiang <[email protected]>
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8567 +/- ##
==========================================
+ Coverage 77.57% 77.65% +0.07%
==========================================
Files 474 474
Lines 62033 62094 +61
==========================================
+ Hits 48122 48218 +96
+ Misses 10355 10334 -21
+ Partials 3556 3542 -14
Flags with carried forward coverage won't be shown. Click here to find out more. |
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
| // IsDiable implements the Scheduler interface. | ||
| func (l *balanceLeaderScheduler) IsDisable() bool { | ||
| return l.conf.isDisable() | ||
| } |
There was a problem hiding this comment.
Do we need to implement it for every scheduler?
There was a problem hiding this comment.
base_scheduler.go implemented it.
There was a problem hiding this comment.
What I mean is can we implement this common function only once.
There was a problem hiding this comment.
If we want to implement it once, we need to put isDisable and setDisable into schedulerConfig interface. Do you think you need to do this?
There was a problem hiding this comment.
WDYT? Is it possible that we might customize it?
There was a problem hiding this comment.
In this pr, we only need to make a customized implementation for defaultScheduler. I think this is acceptable and only needs to be implemented four times.
If follow what you said, it does only need to be implemented once, but I feel that setDisable should not be placed in the schedulerConfig interface, because only defaultScheduler can be setDisabled. But if you insist, I can also modify it
| @@ -52,8 +51,7 @@ const ( | |||
| ) | |||
|
|
|||
| type balanceLeaderSchedulerConfig struct { | |||
| syncutil.RWMutex | |||
| schedulerConfig | |||
| baseDefaultSchedulerConfig | |||
There was a problem hiding this comment.
I suggest moving Disabled here.
There was a problem hiding this comment.
If this, we need implement isDisable and setDisable four times.
| func (*BaseScheduler) SetDisable(bool) error { return nil } | ||
|
|
||
| // IsDefault returns if the scheduler is a default scheduler. | ||
| func (*BaseScheduler) IsDefault() bool { return false } |
There was a problem hiding this comment.
How about:
type hasDefaulSchedulerConfig interface {
func IsDisable() bool
func SetDisable(disabled bool) error
}
func (s *BaseScheduler) IsDefault() bool {
_, ok := s.conf.(hasDefaulSchedulerConfig)
return ok
}
func (s *BaseScheduler) SetDisable(v bool) error {
if c, ok := s.conf.(hasDefaulSchedulerConfig);ok {
c.SetDisable(v)
}
return nil
}
func (*BaseScheduler) IsDisable() bool {
if c, ok := s.conf.(hasDefaulSchedulerConfig);ok {
return c.IsDisable()
}
return false
}
then no need to rewrite the interface for default scheduler.
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
| @@ -621,6 +621,7 @@ func (suite *schedulerTestSuite) checkHotRegionSchedulerConfig(cluster *pdTests. | |||
| "min-hot-key-rate": float64(10), | |||
| "min-hot-query-rate": float64(10), | |||
| "src-tolerance-ratio": 1.05, | |||
| "disabled": false, | |||
There was a problem hiding this comment.
Is it allowed to be set through scheduler config xxx?
There was a problem hiding this comment.
In current pr, it is allowed. Do we need mask it for users?
There was a problem hiding this comment.
I prefer to make it only for internal use purpose.
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
Signed-off-by: okJiang <[email protected]>
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: niubell, nolouch, 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:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: Ref #8474
Before deprecating
scheduler-v2, we need to save thedisablefield of the default scheduler to the etcdxxxx/scheduler_configpath. This pr does not modify the use ofdisable.disableonly is added to/deleted from the storage. So it will not affect the existing logic.What is changed and how does it work?
Check List
Tests
Code changes
Release note