-
Notifications
You must be signed in to change notification settings - Fork 720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
schedulers: add role config to shuffleRegionScheduler #2219
Conversation
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
/run-test |
|
||
func (conf *shuffleRegionSchedulerConfig) ServeHTTP(w http.ResponseWriter, r *http.Request) { | ||
router := mux.NewRouter() | ||
router.HandleFunc("/roles", conf.handleGetRoles).Methods("GET") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the root path of PD's API? Maybe add the PD API prefix is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it is a sub router to be registered under scheduler_config/shuffle-region-scheduler/
.
if cluster.IsPlacementRulesEnabled() { | ||
scoreGuard = filter.NewRuleFitFilter(s.GetName(), cluster, region, oldPeer.GetStoreId()) | ||
} else { | ||
scoreGuard = filter.NewDistinctScoreFilter(s.GetName(), cluster.GetLocationLabels(), cluster.GetRegionStores(region), cluster.GetStore(oldPeer.GetStoreId())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it different from the original behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, different. Priviously it does not consider any constraints.
roles = append(roles, f) | ||
} | ||
} | ||
b, _ := json.Marshal(roles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to handle this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should not return any error as it only contains basic types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
/run-all-tests |
@disksing merge failed. |
/run-test |
/merge |
/run-all-tests |
@disksing merge failed. |
CI failure cased by #2231 |
/merge |
/run-all-tests |
@disksing merge failed. |
/merge |
/run-all-tests |
cherry pick to release-3.1 failed |
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
What problem does this PR solve?
Fix #2177
What is changed and how it works?
Add role configuration to shuffle-region-scheduler.
Use it to balance learners:
1. add shuffle-region-scheduler
or
2. config roles
or
Check List
Tests
Code changes
Related changes