server/balancer: guarantee replicas are safe if possible #473
Conversation
| } | ||
|
|
||
| // selectBestPeer returns the best peer in other stores. | ||
| func (r *replicaChecker) selectBestPeer(region *regionInfo, filters ...Filter) (*metapb.Peer, float64) { | ||
| filters = append(filters, r.filters...) | ||
| // Add some must have filters. | ||
| filters = append(filters, newStateFilter(r.opt)) |
There was a problem hiding this comment.
How about adding a healthFilter?
There was a problem hiding this comment.
healthFilter will be used after the best store has been found, or the best store can be filtered if it is temporarily unavailable.
| if bestStore == nil || compareStoreScore(store, score, bestStore, bestScore) > 0 { | ||
| bestStore = store | ||
| bestScore = score | ||
| } | ||
| } | ||
|
|
||
| if bestStore == nil { | ||
| if bestStore == nil || filterTarget(bestStore, r.filters) { |
There was a problem hiding this comment.
Why not check r.filters along with filters?
There was a problem hiding this comment.
r.filters are used to filter temporarily unavailable stores. We may choose an unsafe store if we filter those temporarily unavailable stores before calculating the score.
|
|
||
| // If store 7 is not available, we wait. | ||
| tc.setStoreDown(7) | ||
| c.Assert(sb.Schedule(cluster), IsNil) |
There was a problem hiding this comment.
Can we balance 1->6 instead if 7 is down?
There was a problem hiding this comment.
It's not safe to balance 1->6, we don't need to balance immediately if 7 is down.
| // If store 7 is not available, we wait. | ||
| tc.setStoreDown(7) | ||
| c.Assert(sb.Schedule(cluster), IsNil) | ||
| c.Assert(sb.cache.get(1), IsTrue) |
There was a problem hiding this comment.
It does not look reasonable to mark 1 as 'not able to balance' when the actual reason is 7 is down.
There was a problem hiding this comment.
Store 1 is not able to balance temporarily when 7 is down, we can skip it for a while.
|
@overvenus Yes, the option has been removed for simplicity. |
|
LGTM |
| if len(v) == 0 { | ||
| return "" | ||
| } | ||
| id += s.getLabelValue(k) |
There was a problem hiding this comment.
I concat the values into an id, so id will be more appropriate.
There was a problem hiding this comment.
what I mean is id += v, no need to call s.getLabelValue(k) twice.
| for _, k := range keys { | ||
| v := s.getLabelValue(k) | ||
| if len(v) == 0 { | ||
| return "" |
There was a problem hiding this comment.
no need to consider the latter key which value is not empty?
There was a problem hiding this comment.
I don't think we should support missing label cases, it's error prone and hard to explain the behavior.
If we want to update the configurations or labels, we can just turn off the balance temporarily.
There was a problem hiding this comment.
So we should forbid the missing label in TiKV, and if TiKV reports the label without a value, we should log an error and don't add this label too.
|
Rest LGTM |
| return cluster.putStore(newStoreInfo(store)) | ||
| // Check location labels. | ||
| for _, k := range c.s.cfg.Replication.LocationLabels { | ||
| if v := s.getLabelValue(k); len(v) == 0 { |
There was a problem hiding this comment.
should check before update the store info?
There was a problem hiding this comment.
That's ok, it is a clone.
We can not do balance safely if we have only 3 data centers, because of tikv/tikv#1468.
So, the current implementation guarantee safety if we have more than 3 data centers, but we will still balance if we have only 3 data cetners.