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

placement: add FitRegion implementation #1904

Merged
merged 6 commits into from
Nov 7, 2019
Merged

Conversation

disksing
Copy link
Contributor

@disksing disksing commented Nov 6, 2019

Signed-off-by: disksing i@disksing.com

What problem does this PR solve?

continue merging placement rules.

What is changed and how it works?

add FitRegion implementation

Check List

Tests

  • Unit test

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

codecov-io commented Nov 6, 2019

Codecov Report

Merging #1904 into master will increase coverage by 0.76%.
The diff coverage is 89.28%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1904      +/-   ##
=========================================
+ Coverage   77.33%   78.1%   +0.76%     
=========================================
  Files         165     167       +2     
  Lines       16590   16613      +23     
=========================================
+ Hits        12830   12975     +145     
+ Misses       2750    2644     -106     
+ Partials     1010     994      -16
Impacted Files Coverage Δ
server/schedule/placement/fit.go 74.8% <89.28%> (+74.8%) ⬆️
server/kv/etcd_kv.go 75.32% <0%> (-6.5%) ⬇️
server/schedulers/random_merge.go 65.45% <0%> (-3.64%) ⬇️
server/schedule/operator/operator.go 85% <0%> (-1.42%) ⬇️
server/member/leader.go 78.57% <0%> (-1.03%) ⬇️
server/grpc_service.go 57.75% <0%> (-0.87%) ⬇️
pkg/btree/btree.go 86.94% <0%> (-0.61%) ⬇️
server/handler.go 53.1% <0%> (-0.5%) ⬇️
server/api/router.go 100% <0%> (ø) ⬆️
server/api/middleware.go 75% <0%> (ø)
... and 22 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 926b46e...014bbd0. Read the comment docs.

Copy link
Contributor

@shafreeck shafreeck left a comment

Choose a reason for hiding this comment

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

Unit tests are really necessary for these utility functions

server/schedule/placement/fit.go Show resolved Hide resolved
@disksing disksing added the component/schedule Scheduling logic. label Nov 7, 2019
Copy link
Contributor

@shafreeck shafreeck left a comment

Choose a reason for hiding this comment

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

LGTM

for _, rule := range rules {
rf := fitRule(peers, rule)
regionFit.RuleFits = append(regionFit.RuleFits, rf)
// Remove selected.
Copy link
Contributor

Choose a reason for hiding this comment

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

does it mean there is only one rule in a range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. There can be multiple rules in a range, i.e. multiple rules for a region.
Remove selected here is because a peer can only belong to a Rule.

server/schedule/placement/fit.go Outdated Show resolved Hide resolved
Signed-off-by: disksing <i@disksing.com>

Co-Authored-By: lhy1024 <admin@liudos.us>
Signed-off-by: disksing <i@disksing.com>
Copy link
Contributor

@nolouch nolouch 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

disksing commented Nov 7, 2019

/merge

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

sre-bot commented Nov 7, 2019

/run-all-tests

@sre-bot sre-bot merged commit d6d08d9 into tikv:master Nov 7, 2019
@disksing disksing deleted the placement9 branch November 7, 2019 09:14
@disksing disksing mentioned this pull request Dec 13, 2019
9 tasks
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.

7 participants