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

*: add adjacent region balance scheduler #780

Merged
merged 29 commits into from Oct 29, 2017

Conversation

Projects
None yet
7 participants
@nolouch
Member

nolouch commented Sep 29, 2017

This scheduler tries to disperse the adjacent region.

  • scan the regions and pick the adjacent regions
  • disperse them to the different store

@nolouch nolouch changed the title from [DNM] *: add adjacent region balance scheduler to *: add adjacent region balance scheduler Oct 17, 2017

@nolouch

This comment has been minimized.

Show comment
Hide comment
@nolouch
Member

nolouch commented Oct 19, 2017

Show outdated Hide outdated server/cache.go
Show outdated Hide outdated server/core/region.go
@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Oct 21, 2017

Member

/run-integration-compatibility-test tidb-test=connor/add-compatible-test

Member

iamxy commented Oct 21, 2017

/run-integration-compatibility-test tidb-test=connor/add-compatible-test

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Oct 21, 2017

Member

/run-all-tests

Member

iamxy commented Oct 21, 2017

/run-all-tests

func (l *balanceAdjacentRegionScheduler) Schedule(cluster schedule.Cluster) *schedule.Operator {
if l.cacheRegions == nil {
l.cacheRegions = &adjacentState{
assignedStoreIds: make([]uint64, 0, len(cluster.GetStores())),

This comment has been minimized.

@disksing

disksing Oct 23, 2017

Member

The stores should be updated everytime Schedule is called. If the namespace feature is enabled, the stores may change.

@disksing

disksing Oct 23, 2017

Member

The stores should be updated everytime Schedule is called. If the namespace feature is enabled, the stores may change.

This comment has been minimized.

@nolouch

nolouch Oct 23, 2017

Member

the assigned store id is the exclude id.

@nolouch

nolouch Oct 23, 2017

Member

the assigned store id is the exclude id.

@disksing

This comment has been minimized.

Show comment
Hide comment
@disksing

disksing Oct 23, 2017

Member

LGTM.

Member

disksing commented Oct 23, 2017

LGTM.

@zhangjinpeng1987

This comment has been minimized.

Show comment
Hide comment
@zhangjinpeng1987
Contributor

zhangjinpeng1987 commented Oct 25, 2017

tc.addLeaderRegionWithRange(5, "e", "f", 1, 2, 3)
tc.addLeaderRegionWithRange(6, "f", "g", 1, 2, 3)
tc.addLeaderRegionWithRange(7, "z", "", 1, 2, 3)

This comment has been minimized.

@huachaohuang

huachaohuang Oct 26, 2017

Member

Can you add some comments about why these regions will move like this?

@huachaohuang

huachaohuang Oct 26, 2017

Member

Can you add some comments about why these regions will move like this?

l.cacheRegions.clear()
maxLen = len(adjacentRegions)
l.cacheRegions.regions = append(l.cacheRegions.regions, adjacentRegions...)
adjacentRegions = adjacentRegions[:0]

This comment has been minimized.

@huachaohuang

huachaohuang Oct 27, 2017

Member

This seems not right, for example, if maxLen = 0, len(adjacentRegions) = 2, then adjacentRegions will be clear, but what if the next region is also adjacent?
BTW, the flow in this for loop seems not straight.

@huachaohuang

huachaohuang Oct 27, 2017

Member

This seems not right, for example, if maxLen = 0, len(adjacentRegions) = 2, then adjacentRegions will be clear, but what if the next region is also adjacent?
BTW, the flow in this for loop seems not straight.

This comment has been minimized.

@nolouch

nolouch Oct 27, 2017

Member

if next region is also adjacent, thelen(adjacentRegions) will be 3 not 2. I mean it will not execute to here because of continue in L174. here we Intention to record max length adjacent regions.

@nolouch

nolouch Oct 27, 2017

Member

if next region is also adjacent, thelen(adjacentRegions) will be 3 not 2. I mean it will not execute to here because of continue in L174. here we Intention to record max length adjacent regions.

This comment has been minimized.

@huachaohuang

huachaohuang Oct 27, 2017

Member

Got it, but it seems not Intuitive :(

@huachaohuang

huachaohuang Oct 27, 2017

Member

Got it, but it seems not Intuitive :(

@disksing disksing added this to the 1.1 milestone Oct 27, 2017

@huachaohuang

LGTM

l.cacheRegions.clear()
maxLen = len(adjacentRegions)
l.cacheRegions.regions = append(l.cacheRegions.regions, adjacentRegions...)
adjacentRegions = adjacentRegions[:0]

This comment has been minimized.

@huachaohuang

huachaohuang Oct 27, 2017

Member

Got it, but it seems not Intuitive :(

@huachaohuang

huachaohuang Oct 27, 2017

Member

Got it, but it seems not Intuitive :(

@huachaohuang

This comment has been minimized.

Show comment
Hide comment
@huachaohuang

huachaohuang Oct 27, 2017

Member

/run-all-test

Member

huachaohuang commented Oct 27, 2017

/run-all-test

@nolouch

This comment has been minimized.

Show comment
Hide comment
@nolouch

nolouch Oct 27, 2017

Member

/run-all-test

Member

nolouch commented Oct 27, 2017

/run-all-test

@nolouch

This comment has been minimized.

Show comment
Hide comment
@nolouch

nolouch Oct 29, 2017

Member

/run-all-test

Member

nolouch commented Oct 29, 2017

/run-all-test

@nolouch nolouch merged commit 98867cf into master Oct 29, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-pd/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@nolouch nolouch deleted the shuning/adjacent-region branch Oct 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment