storage: inline addAndRemoveReplicas into ChangeReplicas

[tbg]

The main contribution here is a complete reworking of the comment at the
top of ChangeReplicas to take into account atomic replication changes. It
calls out that they are not supported yet.

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 6 of 6 files at r1, 6 of 6 files at r2, 2 of 2 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/storage/client_raft_test.go, line 1190 at r1 (raw file):

			t.Fatal(err)
		}
		target := roachpb.ReplicationTarget{

We're inconsistent about whether we inline these structs or not. We might as well do the same thing everywhere.

---

pkg/storage/replica_command.go, line 800 at r3 (raw file):

}

// ChangeReplicas atomically changes the replicas that are a member of a range.

"that are members"

---

pkg/storage/replica_command.go, line 838 at r3 (raw file):

//    explicitly as a convenient way to ensure learners are caught up before the
//    next step is entered. (We ensure that work is not duplicated between the
//    snapshot queue and the explicit snapshot).

How do we ensure this? Can we include that here?

---

pkg/storage/replica_command.go, line 853 at r3 (raw file):

//    is not represented in the RangeDescriptor (which is the source of truth of
//    the replication configuration), additional information about the joint state
//    is persisted in a replicated key located on the range. Raft will

Might as well point out what this key is named (will be named).

---

pkg/storage/replica_command.go, line 875 at r3 (raw file):

// A replica that learns that it was removed will queue itself for replicaGC.
// Note that a removed replica may never apply the configuration change removing
// it and thus this trigger may not fire. This is because said replica may not

nit: s/it/itself/ would make this clearer.

---

pkg/storage/replica_command.go, line 877 at r3 (raw file):

// it and thus this trigger may not fire. This is because said replica may not
// have been a part of the quorum that committed the configuration change; if
// the leader applies it before the removed replica receives it, it will never

nit: there's one too many "it"s in this sentence to make sense on a first pass. Consider re-wording.

[tbg]

TFTR!

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/storage/replica_command.go, line 853 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Might as well point out what this key is named (will be named).

Done.

[tbg]

bors r=nvanbenschoten

[craig]

Build succeeded

• GitHub CI (Cockroach)