storage: prep ChangeReplicasTrigger for atomic rebalancing

[tbg]

The implementation plan for atomic rebalancing is to first expose a
testing-only API that lets us carry out general atomic membership changes
(i.e. not limited to a single swap which is all that we expect to want to
use in production in 19.2).

This allows us to really stress the machinery in general and also prevents
more API headaches down the road should we want to support more general
changes. Besides, the change to an API that supports a single swap only is
already similarly invasive.

This is the first in a sequence of PRs/commits that plumb the API through
the system (but without actually supporting atomic changes yet). The whole
pipeline that needs to be plumbed is

• AdminChangeReplicasRequest which calls into
• (*Replica).ChangeReplicas which calls into
• the internal code adding learners, sending them snapshots, etc, and
  which finally evaluates into a
• ChangeReplicasTrigger that needs to eventually be translated
  into a
• raftpb.ConfChangeV2 (we use a v1 ConfChange today).

This particular change deprecates the Replica and ChangeType field of
the trigger, and introduces instead two slices of ReplicaDescriptors, one
for added and one for removed replicas. This will be easy to translate into
a ConfChangeV2 and seemed more idiomatic (and less error-prone) than
including the base desc and computing a sequence of changes from that
whenever it is needed. It also matches what the higher levels of the API
will do.

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

all nits. Thanks for the consumable change.

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

---

docs/generated/settings/settings.html, line 125 at r2 (raw file):

<tr><td><code>trace.lightstep.token</code></td><td>string</td><td><code></code></td><td>if set, traces go to Lightstep using this token</td></tr>
<tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>if set, traces go to the given Zipkin instance (example: '127.0.0.1:9411'); ignored if trace.lightstep.token is set</td></tr>
<tr><td><code>version</code></td><td>custom validation</td><td><code>19.1-8</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>

nit: it looks like this landed in the wrong commit.

---

pkg/roachpb/data.go, line 1310 at r1 (raw file):

	if crt.Desc != nil {
		nextReplicaID = crt.Desc.NextReplicaID
		// NB: we don't want to mutate InternalReplicas, so we don't call

Will this change once #39489 is merged?

---

pkg/roachpb/data.go, line 1327 at r1 (raw file):

		fmt.Fprintf(&chgS, "%s%s", REMOVE_REPLICA, removed)
	}
	return fmt.Sprintf("%s: after=%s next=%d", chgS.String(), afterReplicas, nextReplicaID)

nit: you might as well keep using the strings.Builder:

fmt.Fprintf(&chgS, ": after=%s next=%d", afterReplicas, nextReplicaID)
return chgS.String()

---

pkg/roachpb/data.proto, line 165 at r1 (raw file):

  option (gogoproto.goproto_stringer) = false;

  // TODO(tbg): remove when possible. 19.2 won't actively use this any more,

Mind mentioning when this will be possible?

---

pkg/roachpb/data.proto, line 189 at r1 (raw file):

  // set when this field is used). Must not specify the same replica more than once or
  // overlap with the added replicas above.
  repeated ReplicaDescriptor internal_removed_replicas = 7 [(gogoproto.nullable) = false];

The plan is to remove the "internal" prefix from these eventually, right?

---

pkg/settings/cluster/cockroach_versions.go, line 509 at r1 (raw file):

	},
	{
		// VersionAtomicChangeReplicasTrigger is <TODO>.

Reminder to fill this in.

---

pkg/storage/replica_raft.go, line 293 at r1 (raw file):

		prefix = false

		// Ensure that we aren't trying to remove ourselves from the range without

It seems strange to me that this lives here. I think I tried moving it down to batch evaluation at one point, but ran into issues because the commit triggers are a little panic-y. If you're working in this area and have it all paged in, do you mind taking a pass at giving this check a better home?

[tbg]

Thank you! All addressed.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/roachpb/data.go, line 1310 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Will this change once #39489 is merged?

Yes! Added a TODO to revisit.

---

pkg/roachpb/data.proto, line 165 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Mind mentioning when this will be possible?

Added a reference to #39182.

---

pkg/roachpb/data.proto, line 189 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The plan is to remove the "internal" prefix from these eventually, right?

Yep, added TODO to the deprecated fields.

---

pkg/storage/replica_raft.go, line 293 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It seems strange to me that this lives here. I think I tried moving it down to batch evaluation at one point, but ran into issues because the commit triggers are a little panic-y. If you're working in this area and have it all paged in, do you mind taking a pass at giving this check a better home?

I share the sentiment, will take another look when I'm less pressed for time. This should be straightforward.

[tbg]

bors r=nvanbenschoten

[craig]

Build succeeded

• GitHub CI (Cockroach)