Skip to content
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

schedule: make region scheduler and checkers aware of placement rules #2051

Merged
merged 7 commits into from
Dec 26, 2019

Conversation

disksing
Copy link
Contributor

What problem does this PR solve?

Continue merge placement rules.

What is changed and how it works?

  • skip strict label match if placement rules enabled
  • disallow merge regions that across different rules
  • replace learnerChecker and replicaChecker with ruleChecker if placement rules enabled
  • replace distinctScoreFilter with RuleFitFilter if placement rules enabled
  • add tests

Check List

Tests

  • Unit test

Signed-off-by: disksing <i@disksing.com>
@disksing disksing added the component/schedule Scheduling logic. label Dec 24, 2019
@disksing disksing added this to the v4.0.0-beta milestone Dec 24, 2019
@codecov-io
Copy link

codecov-io commented Dec 24, 2019

Codecov Report

Merging #2051 into master will increase coverage by <.01%.
The diff coverage is 79.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2051      +/-   ##
==========================================
+ Coverage   76.87%   76.87%   +<.01%     
==========================================
  Files         185      185              
  Lines       18835    18871      +36     
==========================================
+ Hits        14479    14507      +28     
+ Misses       3253     3250       -3     
- Partials     1103     1114      +11
Impacted Files Coverage Δ
server/config/config.go 83.47% <ø> (ø) ⬆️
server/cluster/coordinator.go 78.39% <100%> (ø) ⬆️
server/schedule/checker_controller.go 76.66% <66.66%> (-14.64%) ⬇️
server/schedulers/balance_region.go 84.29% <81.81%> (+2.88%) ⬆️
server/cluster/cluster.go 81.51% <83.33%> (+0.23%) ⬆️
server/schedule/checker/merge_checker.go 70.73% <85.71%> (+1.28%) ⬆️
server/kv/etcd_kv.go 75.32% <0%> (-9.1%) ⬇️
server/tso/tso.go 77.37% <0%> (-4.38%) ⬇️
pkg/etcdutil/etcdutil.go 88.4% <0%> (-2.9%) ⬇️
server/member/leader.go 78.57% <0%> (-1.54%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b93c794...5bb0402. Read the comment docs.

@@ -200,6 +200,13 @@ func (c *RaftCluster) Start(s Server) error {
return nil
}

defaultReplicas := c.opt.GetMaxReplicas()
defaultLocationLabels := c.opt.GetLocationLabels()
c.ruleManager, err = placement.NewRuleManager(c.storage, defaultReplicas, defaultLocationLabels)
Copy link
Member

Choose a reason for hiding this comment

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

When we disable the placement rules, will it affect the start process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is always loaded no matter enabled or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rleungx fixed.

}

func (s *testBalanceRegionSchedulerSuite) checkReplacePendingRegion(c *C, tc *mockcluster.Cluster, opt *mockoption.ScheduleOptions, sb schedule.Scheduler) {

Copy link
Member

Choose a reason for hiding this comment

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

empty line

@disksing disksing mentioned this pull request Dec 25, 2019
9 tasks
Copy link
Contributor

@lhy1024 lhy1024 left a comment

Choose a reason for hiding this comment

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

the rest LGTM

server/schedule/checker_controller.go Outdated Show resolved Hide resolved
Signed-off-by: disksing <i@disksing.com>
@disksing
Copy link
Contributor Author

Please take a look @rleungx

Signed-off-by: disksing <i@disksing.com>
Copy link
Member

@rleungx rleungx left a comment

Choose a reason for hiding this comment

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

LGTM

@disksing
Copy link
Contributor Author

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 26, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 26, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Dec 26, 2019

@disksing merge failed.

@disksing
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 26, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Dec 26, 2019

@disksing merge failed.

@nolouch
Copy link
Contributor

nolouch commented Dec 26, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 26, 2019

/run-all-tests

@sre-bot sre-bot merged commit 6866fda into tikv:master Dec 26, 2019
@disksing disksing deleted the placement14 branch December 27, 2019 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/schedule Scheduling logic. status/can-merge Indicates a PR has been approved by a committer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants