[coordinator] placement: add+remove fully atomic #1022
Conversation
| badInsts := []string{} | ||
| for _, inst := range p.Instances() { | ||
| if inst.Shards().NumShards() == 0 { | ||
| continue |
There was a problem hiding this comment.
I don't think this is possible in valid placement? could you add this to the list of badInsts for now and return as error?
There was a problem hiding this comment.
Hmm if it's not possible in a valid placement I'll revert this. I mainly did it because I had a test with an instance with no shards
There was a problem hiding this comment.
Yeah we are checking against instance with no shards in a sharded placement, so this should not be possible
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return newPlacement, nil | ||
| } | ||
| if err := validateAllAvailable(curPlacement); err != nil { |
There was a problem hiding this comment.
This should work, but I feel a more reusable approach would be passing a placement validation function to the service through options and call it there, in case one day we change the logic in placement service and forgot to update here, what do you think?
There was a problem hiding this comment.
That does sound more flexible, but so far this is the only use case we've had for this and I'd rather limit the scope in this PR to unblock progress.
There was a problem hiding this comment.
Opened m3db/m3cluster#243
Codecov Report
@@ Coverage Diff @@
## master #1022 +/- ##
==========================================
- Coverage 76.72% 76.65% -0.08%
==========================================
Files 436 436
Lines 37002 37037 +35
==========================================
- Hits 28391 28390 -1
- Misses 6585 6616 +31
- Partials 2026 2031 +5
Continue to review full report at Codecov.
|
| badInsts := []string{} | ||
| for _, inst := range p.Instances() { | ||
| if inst.Shards().NumShards() == 0 { | ||
| continue |
There was a problem hiding this comment.
Yeah we are checking against instance with no shards in a sharded placement, so this should not be possible
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return newPlacement, nil | ||
| } | ||
| if err := validateAllAvailable(curPlacement); err != nil { |
I realized that even with the change to make adds "safe" by default, there could still be a race. That is, the placement that all shards were validated as AVAILABLE for could not be the present placement by the time `AddInstances` was called. This changes placement adds/removes to use the placement algorithm to construct the new placement, and perform a `CheckAndSet` with the old placement. Additionally, this implements "safe" adds in the delete handler as well.
schallert
force-pushed
the
schallert/coord_safe_remove
branch
from
ea0b441 to
6dcfaf0
Compare
I realized that even with the change to make adds "safe" by default,
there could still be a race. That is, the placement that all shards were
validated as AVAILABLE for could not be the present placement by the
time
AddInstanceswas called.This changes placement adds/removes to use the placement algorithm to
construct the new placement, and perform a
CheckAndSetwith the oldplacement.
Additionally, this implements "safe" adds in the delete handler as well.