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 rule definitions #1834

Merged
merged 10 commits into from
Oct 29, 2019
Merged

placement: add rule definitions #1834

merged 10 commits into from
Oct 29, 2019

Conversation

disksing
Copy link
Contributor

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

What problem does this PR solve?

Add definitions for placement rules feature.

What is changed and how it works?

cannot work now.

Check List

Tests

  • Unit test

Signed-off-by: disksing <i@disksing.com>
@disksing disksing added the component/schedule Scheduling logic. label Oct 23, 2019
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.

rest LGTM

server/schedule/placement/label_constraint.go Show resolved Hide resolved
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
@codecov-io
Copy link

codecov-io commented Oct 28, 2019

Codecov Report

Merging #1834 into master will decrease coverage by 0.05%.
The diff coverage is 86.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1834      +/-   ##
==========================================
- Coverage    78.1%   78.05%   -0.06%     
==========================================
  Files         168      169       +1     
  Lines       17019    16975      -44     
==========================================
- Hits        13293    13249      -44     
- Misses       2644     2653       +9     
+ Partials     1082     1073       -9
Impacted Files Coverage Δ
pkg/slice/slice.go 100% <100%> (ø)
server/schedule/placement/label_constraint.go 80% <80%> (ø)
server/schedule/placement/rule.go 87.87% <87.87%> (ø)
server/core/storage.go 69.46% <0%> (-6.86%) ⬇️
server/schedulers/random_merge.go 58.13% <0%> (-4.66%) ⬇️
server/region_syncer/server.go 77.77% <0%> (-4.28%) ⬇️
pkg/mock/mockcluster/mockcluster.go 90.2% <0%> (-1.75%) ⬇️
server/grpc_service.go 58.78% <0%> (-0.66%) ⬇️
client/client.go 69.89% <0%> (-0.44%) ⬇️
server/schedule/operator_controller.go 90.9% <0%> (-0.17%) ⬇️
... and 20 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 628ac1b...6f16210. Read the comment docs.

Signed-off-by: disksing <i@disksing.com>
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/placement/label_constraint.go Outdated Show resolved Hide resolved
server/schedule/placement/label_constraint.go Outdated Show resolved Hide resolved
server/schedule/placement/label_constraint.go Show resolved Hide resolved
Signed-off-by: disksing <i@disksing.com>

Co-Authored-By: lhy1024 <admin@liudos.us>
// applying rules (apply means schedule regions to match selected rules), the
// apply order is defined by the tuple [GroupID, Index, ID].
type Rule struct {
GroupID string `json:"group_id"` // mark the source that add the rule
Copy link
Member

Choose a reason for hiding this comment

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

BTW, I think it's better to unify the style of it in the future. We have both snake_case and kebab-case in our repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

em, so are you suggesting using kebab style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can decide later, now both styles are used commonly.

Copy link
Member

Choose a reason for hiding this comment

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

Of course.

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 disksing added the status/can-merge Indicates a PR has been approved by a committer. label Oct 29, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 29, 2019

/run-all-tests

@sre-bot sre-bot merged commit f7dae8b into tikv:master Oct 29, 2019
@disksing disksing deleted the placement1 branch October 29, 2019 08:23
@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.

6 participants