Work towards #2152, Increasing transactional safety of replica changes #2241
Conversation
|
great PR name |
|
Sorry, I haven't gotten used to that part of the multi-commit workflow (re: PR title) |
mrtracy
changed the title
| @@ -594,10 +592,10 @@ func (r *Replica) addAdminCmd(ctx context.Context, args proto.Request) (proto.Re | |||
|
|
|||
| switch tArgs := args.(type) { | |||
| case *proto.AdminSplitRequest: | |||
| resp, err := r.AdminSplit(*tArgs) | |||
| resp, err := r.AdminSplit(*tArgs, r.Desc()) | |||
There was a problem hiding this comment.
Shouldn't this get passed in as part of the AdminSplitRequest? Otherwise, you still have a race condition. You want to be sure the split proceeds only if the range descriptor is unchanged from its value when the split was decided. Same for merge of course.
There was a problem hiding this comment.
I actually mentioned this in the commit message; this pathway should only be used if an admin is manually splitting a range, internal pathways (e.g. "maybeSplitOnZoneConfig" or whatever) should just call AdminSplit directly.
However, i'm saving that change for a future commit - i'll create an issue for it.
There was a problem hiding this comment.
Unless you think it's better just to put the RangeDescriptor into the AdminSplitRequest proto.
There was a problem hiding this comment.
+1 internal pathways calling AdminSplit directly
|
LGTM |
|
There was some open feedback on this, is everyone satisfied? |
mrtracy
force-pushed
the
mtracy-replica-change-safety
branch
from
0b6f64d to
b3b6d9b
Compare
For issue cockroachdb#2152 This mutex was providing no real concurrent safety, because it gave no consideration to actions by other stores. Concurrent safety is already provided by the transactions in each operation, so it can be removed.
For cockroachdb#2152 This commit modifies AdminSplit, AdminMerge and ChangeReplicas to accept a RangeDescriptor object. The RangeDescriptor is used as a form of optimistic locking. All of these operations will ultimately modify the RangeDescriptor in some way; to ensure that no other concurrent operations modify the RangeDescriptor, a copy of the original RangeDescriptor is captured before the modified RangeDescriptor is computed. The original RangeDescriptor is passed to a ConditionalPut as the first operation in the transaction, causing it to fail if concurrent modifications have already committed. However, capturing the original RangeDescriptor inside of the Split/Merge/Change methods is insufficient. In most cases, the decision to call Split/Merge/Change is made by another method based on the RangeDescriptor, and the decision may have been different if a concurrent change is applied first. Therefore, the original RangeDescriptor needs to be captured before calling Split/Merge/Change. This commit modifies all three methods to use the new pattern, with a required RangeDescriptor being passed to the method. Note that the behavior of Split and Merge is still not optimal, because the call to AdminSplit or AdminMerge is always routed through Raft. This pattern is unnecessary in cases where the split/merge decision is computed on the server.
mrtracy
force-pushed
the
mtracy-replica-change-safety
branch
from
b3b6d9b to
9ffbd1a
Compare
Work towards #2152, Increasing transactional safety of replica changes
Two commits relevant to #2152