placement: support batch deletions, with insertions #2699
Conversation
ti-srebot
added
the
contribution
| 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 |
There was a problem hiding this comment.
It is admirable for you to notice this issue! I think we can add some note in the API annotation.
|
|
||
| // 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.
I have to say Batch here is a bit of misleading as one Batch only contains one rule.
There was a problem hiding this comment.
Any good idea? RuleOp?
There was a problem hiding this comment.
RuleOp is ok to me. one Batchcontains multiple RuleOp seems more reasonable.
why cannot ensure atomic? |
|
I think the contributor have explained on the following. @disksing // 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 |
| 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 |
There was a problem hiding this comment.
If a transaction API is needed, please create issue to track this (ignore me if already existed).
There was a problem hiding this comment.
This is unexpected. Anyway, I think it is enough to use my old issue to track it.
|
@Yisaer,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: scheduling(slack). |
1 similar comment
|
@Yisaer,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: scheduling(slack). |
|
LGTM |
|
/merge |
ti-srebot
added
the
status/can-merge
|
/run-all-tests |
What problem does this PR solve?
Related to #2680.
Now one can post an array of operations(insert/delete) to
/config/rules/batchin one request. In case there is an error, all modifications are cancelled.Two notable points:
Check List
Tests
Code changes
Release note
/config/rules/batcchfor batch(insert/delete) rules in one request