-
Notifications
You must be signed in to change notification settings - Fork 714
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
server/constraints: add replication constraints #402
Conversation
ae3b984
to
bc8e554
Compare
3751016
to
a939d22
Compare
3c2c3e7
to
4d5ae40
Compare
a939d22
to
501e53c
Compare
should check constraints validation. E,g. if we have max 3 replicas, but the total replicas number > 3, we may panic. |
9ca675f
to
21a0191
Compare
PTAL @siddontang @overvenus |
// Although we can use a more optimize algorithm, but it may be hard | ||
// to understand by most users. | ||
// So we decide to use a straight forward algorithm to match stores | ||
// and constraints one by one in order, and it works find in most |
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.
it works fine
Maybe we should support the exclusive labels like: If two constraints have same value for this label, no matter other label values are different, they are still not exclusive. |
return nil, errors.Errorf("invalid constraint %q", label) | ||
} | ||
k, v := kv[0], kv[1] | ||
if !validLabel.MatchString(k) || !validLabel.MatchString(v) { |
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.
check key, value separately in two if
.
// Match returns true if store matches the constraint. | ||
func (c *Constraint) Match(store *storeInfo) bool { | ||
labels := make(map[string]string) | ||
for _, label := range store.GetLabels() { |
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.
why not checking len(store.GetLabels() == len(c.Labels)
first, then:
for _, label := range store.GetLabels() {
if v := c.Labels[label.GetKey()); v != label.GetValue() {
return false
}
}
So we don't need using another map here.
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.
But the store labels don't need to be exactly the same as the constraint, we just require that the store contains all labels in the constraint.
For example, if the constraint is zone=us
, a store with labels zone=us,disk=ssd
can match the constraint.
return r.removePeer(region, peer) | ||
} | ||
|
||
// Now we have redundant replicas, we can remove unmatched peers. |
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.
do we need to consider redundant but matched replicas here?
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.
There are no such cases. If a constraint requires 2 replicas, then even if we have 3 replicas can match the constraint, the third replicas will not show up in the result, and we will consider that it is redundant.
I don't understand the comment about exclusive labels, can you give me an example? |
E.g, Store A: zone=1 disk=ssd zone is an exclusive label, so if a region has already covered A, we can't use B for it. |
That feature will not included in this PR, we will design and support it later. |
LGTM |
PTAL @overvenus |
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
{"abc=123*"}, | ||
{".123=-abc"}, | ||
{"abc,123=123.abc"}, | ||
{"abc=123", "abc=abc"}, |
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.
Is k= v
a valid lable? (A space after =
)
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.
No, that's not valid.
@@ -70,6 +71,9 @@ type Config struct { | |||
|
|||
MetricCfg MetricConfig `toml:"metric" json:"metric"` | |||
|
|||
ConstraintCfg []ConstraintConfig `toml:"constraints" json:"constraints"` |
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.
Please add some examples in config.toml.
checkAddPeer(c, rc.Check(region), 3) | ||
peer3, _ := cluster.allocPeer(3) | ||
region.Peers = append(region.Peers, peer3) | ||
log.Debug(region) |
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.
Does it help debug? If not, please remove it.
o := &scheduleOption{} | ||
o.store(cfg) | ||
o.store(&cfg.ScheduleCfg) | ||
o.constraints = cfg.constraints |
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.
Potential data race? Why not swap line 345 and 346?
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.
Constraints are read only and will not be changed dynamically, so we don't need to protect it.
Please merge master in order to run tests in circleci. |
I think the score things are too complicated, we can calculate the scores more easily.
I think the score things are too complicated, we can calculate the scores more easily.
Coordinator can control the speed of different schedulers. Every scheduler has a unique name, we can add API to run or stop any schedulers dynamically, and different schedulers can run concurrently.
I think the score things are too complicated, we can calculate the scores more easily.
e0f0ae0
to
e9bf3bb
Compare
* server/selector: add selector interface and balance selector (#388) We can use selector to abstract different strategies to select the source and target stores. * server: use selector to schedule region peer (#389) * server/resource: replace score type with resource kind (#393) I think the score things are too complicated, we can calculate the scores more easily. * server/metrics: remove redundant metrics (#395) * server/balancer: add scheduler and simplify schedule configs (#396) The original balance configs are complicated and some of them are very confusing, now we simplify them to make it more easy to understand. We replace the concept of "balance" with "schedule". Balance means to keep the resource balance, but we will introduce different kinds of strategies in the future which may not balance, and balance is just one kind of strategies to schedule resources between stores. * server/coordinator: replace balancer worker with coordinator (#398) Coordinator can control the speed of different schedulers. Every scheduler has a unique name, we can add API to run or stop any schedulers dynamically, and different schedulers can run concurrently. * server/constraints: add replication constraints (#402) * server/scheduler: add grant-leader-scheduler (#406) * server/coordinator: combine scheduler and controller * server/api: add scheduler api (#407) We can now use API to list all schedulers, add or remove a scheduler. * Add shuffle-leader-scheduler (#409) * server/scheduler: add shuffle-leader-scheduler
This is an example constraints:
We need to ensure all constraints will be satisfied in the replica checker, and maintain the constraints when scheduling peers.