kvserver: remove kv.atomic_replication_changes.enabled setting#61170
Conversation
|
This change is |
c03ae29 to
a213209
Compare
1ef691e to
f44d179
Compare
nvb
left a comment
There was a problem hiding this comment.
Reviewed 5 of 7 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15)
pkg/kv/kvserver/allocator.go, line 888 at r1 (raw file):
zero := roachpb.ReplicationTarget{} // We're going to add another replica to the range which will change the
I don't understand why this allocator change is needed/allowed. Is it no longer applicable now that atomic replication changes are always enabled?
pkg/kv/kvserver/replica.go, line 91 at r1 (raw file):
// This has no effect until the cluster version is 19.2 or higher. var UseAtomicReplicationChanges = settings.RegisterBoolSetting( "kv.atomic_replication_changes.enabled",
We'll want to add this to the retiredSettings map.
This setting was added in 19.2 to provide a fallback against atomic replication changes. They've now been a part of CockroachDB for over 3 releases. They're also a requirement for non-voting replicas. Release justification: removing old cluster setting incompatible with 21.1 features Release note (backward-incompatible change): Removed the `kv.atomic_replication_changes.enabled` cluster setting. All replication changes on a range now use joint-consensus.
f44d179 to
f65313e
Compare
aayushshah15
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/allocator.go, line 888 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I don't understand why this allocator change is needed/allowed. Is it no longer applicable now that atomic replication changes are always enabled?
I think this block of code was only needed when atomic replication changes were disabled, but it was instead used even with them turned on.
I think this bit of code should explain the purpose of this safeguard:
if numLiveReplicas < newQuorum {
// Don't rebalance as we won't be able to make quorum after the rebalance
// until the new replica has been caught up.
return zero, zero, "", false
}
With atomic replication changes, we know we're not going to affect the quorum size during rebalancing so the safeguard provided by this block of code is not needed.
pkg/kv/kvserver/replica.go, line 91 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
We'll want to add this to the
retiredSettingsmap.
Thanks for the pointer, done.
nvb
left a comment
There was a problem hiding this comment.
Reviewed 2 of 7 files at r1, 1 of 1 files at r2.
Reviewable status:
|
TFTR! bors r+ |
|
Build succeeded: |
This setting was added in 19.2 to provide a fallback against atomic
replication changes. They've now been a part of CockroachDB for over 3
releases. They're also a requirement for non-voting replicas.
Release note (backward-incompatible change): Removed the
kv.atomic_replication_changes.enabledcluster setting. All replicationchanges on a range now use joint-consensus.