Skip to content

scheduler: decouple the keyranges with different schedulers #9330

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

bufferflies
Copy link
Contributor

@bufferflies bufferflies commented May 15, 2025

What problem does this PR solve?

Issue Number: Close #9302

What is changed and how does it work?

Check List

Tests

  • Unit test
  • Integration test

Code changes

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

Release note

None.

Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 15, 2025
Copy link
Contributor

ti-chi-bot bot commented May 15, 2025

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

@ti-chi-bot ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 15, 2025
@bufferflies bufferflies force-pushed the feature/decouple_keyrange branch 2 times, most recently from 7fa0925 to 808b7a1 Compare May 16, 2025 03:40
Signed-off-by: 童剑 <[email protected]>
@bufferflies bufferflies force-pushed the feature/decouple_keyrange branch from 808b7a1 to dbff105 Compare May 16, 2025 03:46
Copy link

codecov bot commented May 16, 2025

Codecov Report

Attention: Patch coverage is 95.27027% with 7 lines in your changes missing coverage. Please review.

Project coverage is 76.14%. Comparing base (506cae1) to head (1aa9b40).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9330      +/-   ##
==========================================
+ Coverage   76.02%   76.14%   +0.11%     
==========================================
  Files         475      479       +4     
  Lines       74080    74636     +556     
==========================================
+ Hits        56320    56830     +510     
- Misses      14241    14275      +34     
- Partials     3519     3531      +12     
Flag Coverage Δ
unittests 76.14% <95.27%> (+0.11%) ⬆️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: 童剑 <[email protected]>
@bufferflies bufferflies force-pushed the feature/decouple_keyrange branch from 885ffe7 to 51f06d1 Compare May 22, 2025 12:24
@bufferflies bufferflies force-pushed the feature/decouple_keyrange branch from 51f06d1 to 01f2334 Compare May 27, 2025 07:48
@bufferflies bufferflies marked this pull request as ready for review May 27, 2025 07:48
@ti-chi-bot 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 May 27, 2025
@@ -429,7 +429,12 @@ func makeInfluence(op *operator.Operator, plan *solver, usedRegions map[uint64]s
// It randomly selects a health region from the source store, then picks
// the best follower peer and transfers the leader.
func (s *balanceLeaderScheduler) transferLeaderOut(solver *solver, collector *plan.Collector) *operator.Operator {
solver.Region = filter.SelectOneRegion(solver.RandLeaderRegions(solver.sourceStoreID(), s.conf.getRanges()),
rs := s.conf.getRanges()
if len(rs) == 1 && len(rs[0].StartKey) == 0 && len(rs[0].EndKey) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a function in rs to check len(rs) == 1 && len(rs[0].StartKey) == 0 && len(rs[0].EndKey) == 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Signed-off-by: 童剑 <[email protected]>
@@ -137,6 +137,11 @@ func (s *balanceRegionScheduler) Schedule(cluster sche.SchedulerCluster, dryRun
solver.Step++
var sourceIndex int

rs := s.conf.Ranges
Copy link
Member

@rleungx rleungx May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the Ranges always have a value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it can be seen as

func getKeyRanges(args []string) ([]keyutil.KeyRange, error) {
var ranges []keyutil.KeyRange
for len(args) > 1 {
startKey, err := url.QueryUnescape(args[0])
if err != nil {
return nil, errs.ErrQueryUnescape.Wrap(err)
}
endKey, err := url.QueryUnescape(args[1])
if err != nil {
return nil, errs.ErrQueryUnescape.Wrap(err)
}
args = args[2:]
ranges = append(ranges, keyutil.NewKeyRange(startKey, endKey))
}
if len(ranges) == 0 {
return []keyutil.KeyRange{keyutil.NewKeyRange("", "")}, nil
}
return ranges, nil
}

)

// KeyRangeManager is a manager for key ranges.
type KeyRangeManager struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems keyutil is a more proper package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to the new pkg pkg/schedule/rangelist

@@ -429,7 +429,12 @@ func makeInfluence(op *operator.Operator, plan *solver, usedRegions map[uint64]s
// It randomly selects a health region from the source store, then picks
// the best follower peer and transfers the leader.
func (s *balanceLeaderScheduler) transferLeaderOut(solver *solver, collector *plan.Collector) *operator.Operator {
solver.Region = filter.SelectOneRegion(solver.RandLeaderRegions(solver.sourceStoreID(), s.conf.getRanges()),
rs := s.conf.getRanges()
if IsDefaultKeyRange(rs) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about the hot region scheduler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hot region scheduler can schedule all the regions, not restricting this limit.

Signed-off-by: 童剑 <[email protected]>
Signed-off-by: 童剑 <[email protected]>
@bufferflies bufferflies force-pushed the feature/decouple_keyrange branch from 1aa9b40 to 0e2a025 Compare June 5, 2025 08:18
@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 9, 2025
Copy link
Contributor

ti-chi-bot bot commented Jun 9, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lhy1024

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

ti-chi-bot bot commented Jun 9, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-06-09 03:02:19.409732585 +0000 UTC m=+237717.638047833: ☑️ agreed by lhy1024.

@ti-chi-bot ti-chi-bot bot added the approved label Jun 9, 2025
@bufferflies
Copy link
Contributor Author

ping @rleungx again

for _, r := range rs {
s.sortedKeyRanges.Append(r.StartKey, r.EndKey)
}
s.sortedKeyRanges.SortAndDeduce()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For every time we append, we need to sort?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor Author

@bufferflies bufferflies Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, using adjust function to check the two regions

Signed-off-by: 童剑 <[email protected]>
@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 10, 2025
@@ -41,6 +41,36 @@ func MinKey(a, b []byte) []byte {
return a
}

// MaxKeyWithBoundary returns the bigger key for the given keys.
// it is different from MaxKey, if the key is empty and the boundary is right, the empty key is biggest,
func MaxKeyWithBoundary(a, b []byte, opt boundary) []byte {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about changing to MaxStartKey/MinEndKey? The boundary is useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea

for i := 1; i < len(rs.krs); i++ {
last := res[len(res)-1]
if last.IsAdjacent(rs.krs[i]) {
last.StartKey = MinKeyWithBoundary(last.StartKey, rs.krs[i].StartKey, left)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has already been sorted, no need to compare?

rs := s.conf.getRanges()
if IsDefaultKeyRange(rs) {
km := solver.GetKeyRangeManager()
rs = km.GetNonOverlappingKeyRanges(&rs[0])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it slow down the balance leader's speed?

@@ -137,6 +137,11 @@ func (s *balanceRegionScheduler) Schedule(cluster sche.SchedulerCluster, dryRun
solver.Step++
var sourceIndex int

rs := s.conf.Ranges
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we change the range manually?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved dco-signoff: yes Indicates the PR's author has signed the dco. needs-1-more-lgtm Indicates a PR needs 1 more LGTM. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scheduler: decouple the keyranges with different schedulers
3 participants