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: support batch deletions, with insertions #2699
Changes from 11 commits
0f15f29
f28b7df
b265fb6
d7f17c4
89adfd1
b22d76e
179c208
1be7c34
4ee7943
e0d77ff
97f407d
1637e28
e195768
bdad58d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ import ( | |
"encoding/hex" | ||
"encoding/json" | ||
"fmt" | ||
"strings" | ||
"sync" | ||
|
||
"github.com/pingcap/log" | ||
|
@@ -275,10 +276,51 @@ func (m *RuleManager) FitRegion(stores StoreSet, region *core.RegionInfo) *Regio | |
return FitRegion(stores, region, rules) | ||
} | ||
|
||
func (m *RuleManager) swapRule(rule *Rule) *Rule { | ||
func (m *RuleManager) tryBuildSave(oldRules map[[2]string]*Rule) error { | ||
ruleList, err := buildRuleList(m.rules) | ||
if err == nil { | ||
for key := range oldRules { | ||
rule := m.rules[key] | ||
if rule != nil { | ||
err = m.store.SaveRule(rule.StoreKey(), rule) | ||
} else { | ||
r := Rule{ | ||
GroupID: key[0], | ||
ID: key[1], | ||
} | ||
err = m.store.DeleteRule(r.StoreKey()) | ||
} | ||
if err != nil { | ||
// TODO: it is not completely safe | ||
// 1. in case that half of rules applied, error.. we have to cancel persisted rules | ||
// but that may fail too, causing memory/disk inconsistency | ||
// either rely a transaction API, or clients to request again until success | ||
// 2. in case that PD is suddenly down in the loop, inconsistency again | ||
// now we can only rely clients to request again | ||
break | ||
Comment on lines
+293
to
+300
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a transaction API is needed, please create issue to track this (ignore me if already existed). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unexpected. Anyway, I think it is enough to use my old issue to track it. |
||
} | ||
} | ||
} | ||
|
||
if err != nil { | ||
for key, rule := range oldRules { | ||
if rule == nil { | ||
delete(m.rules, key) | ||
} else { | ||
m.rules[key] = rule | ||
} | ||
} | ||
return err | ||
} | ||
|
||
m.ruleList = ruleList | ||
return nil | ||
} | ||
|
||
func (m *RuleManager) addRule(rule *Rule, oldRules map[[2]string]*Rule) { | ||
old := m.rules[rule.Key()] | ||
m.rules[rule.Key()] = rule | ||
return old | ||
oldRules[rule.Key()] = old | ||
} | ||
|
||
// SetRules inserts or updates lots of Rules at once. | ||
|
@@ -296,31 +338,86 @@ func (m *RuleManager) SetRules(rules []*Rule) error { | |
oldRules := make(map[[2]string]*Rule) | ||
|
||
for _, rule := range rules { | ||
oldRules[rule.Key()] = m.swapRule(rule) | ||
m.addRule(rule, oldRules) | ||
} | ||
|
||
ruleList, err := buildRuleList(m.rules) | ||
if err == nil { | ||
for _, rule := range rules { | ||
err = m.store.SaveRule(rule.StoreKey(), rule) | ||
if err != nil { | ||
break | ||
if err := m.tryBuildSave(oldRules); err != nil { | ||
return err | ||
} | ||
|
||
log.Info("placement rule updated", zap.String("rules", fmt.Sprint(rules))) | ||
return nil | ||
} | ||
|
||
func (m *RuleManager) delRuleByID(group, id string, oldRules map[[2]string]*Rule) { | ||
key := [2]string{group, id} | ||
old, ok := m.rules[key] | ||
if ok { | ||
delete(m.rules, key) | ||
} | ||
oldRules[key] = old | ||
} | ||
|
||
func (m *RuleManager) delRule(t *Batch, oldRules map[[2]string]*Rule) { | ||
if !t.DeleteByIDPrefix { | ||
m.delRuleByID(t.GroupID, t.ID, oldRules) | ||
} else { | ||
for key := range m.rules { | ||
if key[0] == t.GroupID && strings.HasPrefix(key[1], t.ID) { | ||
m.delRuleByID(key[0], key[1], oldRules) | ||
} | ||
} | ||
} | ||
} | ||
|
||
if err != nil { | ||
for key, rule := range oldRules { | ||
if rule == nil { | ||
delete(m.rules, key) | ||
} else { | ||
m.rules[key] = rule | ||
// BatchAction indicates the operation type | ||
type BatchAction string | ||
|
||
const ( | ||
// BatchAdd a placement rule, only need to specify the field *Rule | ||
BatchAdd BatchAction = "add" | ||
// BatchDel a placement rule, only need to specify the field `GroupID`, `ID`, `MatchID` | ||
BatchDel BatchAction = "del" | ||
) | ||
|
||
// Batch is for batching placement rule actions. The action type is | ||
// distinguished by the field `Action`. | ||
type Batch struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have to say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any good idea? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
*Rule // information of the placement rule to add/delete | ||
Action BatchAction `json:"action"` // the operation type | ||
DeleteByIDPrefix bool `json:"delete_by_id_prefix"` // if action == delete, delete by the prefix of id | ||
} | ||
|
||
// Batch executes a series of actions at once. | ||
func (m *RuleManager) Batch(todo []Batch) error { | ||
for _, t := range todo { | ||
switch t.Action { | ||
case BatchAdd: | ||
err := m.adjustRule(t.Rule) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
} | ||
|
||
m.Lock() | ||
defer m.Unlock() | ||
|
||
oldRules := make(map[[2]string]*Rule) | ||
|
||
for _, t := range todo { | ||
switch t.Action { | ||
case BatchAdd: | ||
m.addRule(t.Rule, oldRules) | ||
case BatchDel: | ||
m.delRule(&t, oldRules) | ||
} | ||
} | ||
|
||
if err := m.tryBuildSave(oldRules); err != nil { | ||
return err | ||
} | ||
|
||
m.ruleList = ruleList | ||
log.Info("placement rule updated", zap.String("rules", fmt.Sprint(rules))) | ||
log.Info("placement rules updated", zap.String("batch", fmt.Sprint(todo))) | ||
return nil | ||
} |
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 is admirable for you to notice this issue! I think we can add some note in the API annotation.