-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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: make Constraints can use dict format #47344
Conversation
23007e2
to
136736a
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #47344 +/- ##
================================================
+ Coverage 72.1485% 72.7635% +0.6150%
================================================
Files 1354 1375 +21
Lines 401261 407634 +6373
================================================
+ Hits 289504 296609 +7105
+ Misses 92452 92209 -243
+ Partials 19305 18816 -489
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: nolouch <nolouch@gmail.com>
136736a
to
2b6402d
Compare
/test unit-test |
@nolouch: The specified target(s) for
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
} | ||
rules, err = NewRulesWithDictConstraints(role, cnstr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just use error from NewRulesWithDictConstraints
, instead of Unmarshal
before. Retuning ErrInvalidConstraintsFormat
and wrap the error in NewRules
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here do precheck to ensure that it is in the actual dict
format, then wrap two errors in NewRules
or return dict constraints error(definitely it's dict format), it's easier to handle errors. otherwise, we need to unwrap errors from NewRulesWithDictConstraints
and handle the error to remove the repeated string in the error message.
70e3dc6
to
3205675
Compare
/retest-required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest LGTM
I found some info in docs https://docs.pingcap.com/tidb/stable/placement-rules-in-sql.
You can either specify constraints in list format ([+disk=ssd]) or in dictionary format ({+disk=ssd: 1,+disk=nvme: 2}).
It seems support dictionary format?
ddl/placement/bundle.go
Outdated
} | ||
if len(followerConstraints) == 0 { | ||
if followerCount > 0 { | ||
return nil, fmt.Errorf("%w: specify follower count without specify follower constraints", ErrInvalidPlacementOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about " specify follower count without specify follower constraints when specify other constraints"?
because we can specify follower count only.
@CabinfeverB PTAL again |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CabinfeverB, xhebox 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 |
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: close #47254
Problem Summary:
The customer wants to define a cluster with 5 replicas and distributed in 3 regions, leader can be in every region. currently, the rule in SQL cannot conver it.
What is changed and how it works?
supports the dict format for CONSTRAINTS
Check List
Tests
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.