storage: prepare for kv.atomic_replication_changes=true #40370

This allows simplifying the code because we get to use crt.Desc anywhere we know the trigger was created locally (which is almost everywhere). Release note: None

We don't support removing multiple learners atomically just yet, though \cockroachdb#40268 will fix this (likely in raft). That PR though is obstructed by cockroachdb#40207 because we'll need that first to be able to bump raft again (assuming we don't want to fork). Instead of dealing with all that upfront, let's just not remove multiple learners at once right now so that we can flip the default for atomic replication changes to on. If anyone is still trying to remove only learners atomically, they will fail. However the replicate queues place a high priority on removing stray learners whenever it finds them, so this wouldn't be a permanent problem. Besides, we never add multiple learners at once so it's difficult to get into that state in the first place. Without this commit, TestLearnerAdminRelocateRange fails once atomic replication changes are enabled. Release note: None

This separates concerns nicely, but it's also a required refactor to be more nuanced about the descriptor changing out from under us. Release note: None

This will allow simplifying the implementation by loading the descriptor from KV (instead of using r.Desc() which makes an assumption that the caller picked the right `r`), and it's already mostly true today. (There are no remote production callers of AdminMerge, so no migration concerns). Release note: None

This is in preparation for not loading the descriptor from `r.Desc()` but using conditionalGetDescValueFromDB to obtain it. Release note: None

We'll soon need to allocate the RangeID only once, but potentially create the descriptor multiple times (for each split txn restart). Release note: None

Prepare for getting an updated descriptor handed back from conditionalGetDescValueFromDB in each txn attempt. Release note: None

We're not going to allow parallel writes anytime soon. Release note: None

I don't think those have been useful in a long time. Release note: None

That way it will be easier to avoid confusion between all of the descriptors floating around. Release note: None

Release note: None

It now takes a closure that gets to decide whether the descriptor returned from KV is "acceptable". This descriptor is then returned, and in turn the method doesn't accept a descriptor, just a key from which to look up the descriptor. The point of all this is to allow different callers to check different things. For instance, a split doesn't care whether the set of replicas changed, and a replication change shouldn't fail if the range has since split. Release note: None

We only want to return early on the "stop after learners" knob if we actually did add learners (and not, for example, on a replica removal). Release note: None

ChangeReplicas (and AdminSplit, and AdminMerge) take a RangeDescriptor that they use as a basis for a CPut to make sure the operations mutating the range serialize. This is great for correctness but generally unfortunate for usability since on a mismatch, the caller usually wanted to do the thing they were trying to do anyway, using the new descriptor. The fact that every split (replication change, merge) basically needs a retry loop is constant trickle of test flakes and UX papercuts. It became more pressing to do something against this as we are routinely using joint configs when atomic replication changes are enabled. A joint configuration is transitioned out of opportunistically whenever it is encountered, but this frequently causes a race in which actor A finds a joint config, begins a transaction out of it but is raced by actor B getting there first. The end result is that what actor A wanted to achieve has been achieved, though by someone else, and the result is a spurious error. This commit fixes that behavior in the targeted case of wanting to leave a joint configuration: actor A will get a successful result. Before this change, make stress PKG=./pkg/sql TESTS=TestShowRangesWithLocal would fail immediately when `kv.atomic_replication_changes.enabled` was true because the splits this test carries out would run into the joint configuration changes of the initial upreplication, and would race the replicate queue to transition out of them, which at least one split would typically lose. This still happens, but now it's not an error any more. I do think that it makes sense to use a similar strategy in general (fail replication changes only if the *replicas* changed, allow all splits except when the split key moves out of the current descriptor, etc) but in the process of coding this up I got reminded of all of the problems relating to range merges and also found what I think is a long-standing pretty fatal bug, cockroachdb#40367, so I don't want to do anything until the release is out of the door. But I'm basically convinced that if we did it, it wouldn't cause a new "bug" because any replication change carried out in that way is just one that could be triggered just the same by a user under the old checks. Release note: None

This hopefully makes it easier to reason about the code. Release note: None

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: prepare for kv.atomic_replication_changes=true #40370

storage: prepare for kv.atomic_replication_changes=true #40370

Commits on Sep 4, 2019