roachpb: stop mutating RangeDescriptor when Replicas is called

[danhhz]

It was previously sorting the InternalReplicas field so we could serve
calls to Voters and Learners without allocating, but non-obviousness of
this is not worth it.

Release note: None

[cockroach-teamcity]

This change is 

[danhhz]

Thoughts? We could go even further with the fastpath and return a slice of the wrapped replicas whenever the voters or learners are all consecutive (and then make this more likely by sorting at the end of both AddReplica and RemoveReplica).

[jeffrey-xiao]

If we're removing the sort, we can also remove this hacky fix:

cockroach/pkg/storage/replica_command.go

Lines 1512 to 1527 in b320ff5

	// TODO(jeffreyxiao): This hacky fix ensures that we don't fail the
	// conditional get because of the ordering of InternalReplicas. Calling
	// Replicas() will sort the list of InternalReplicas as a side-effect. The
	// invariant of having InternalReplicas sorted is not maintained in 19.1.
	// Additionally, in 19.2, it's possible for the in-memory copy of
	// RangeDescriptor to become sorted from a call to Replicas() without
	// updating the copy in kv. These two factors makes it possible for the
	// in-memory copy to be out of sync from the copy in kv. The sorted invariant
	// of InternalReplicas is used by ReplicaDescriptors.Voters() and
	// ReplicaDescriptors.Learners().
	if existingDesc != nil {
	existingDesc.Replicas() // for sorting side-effect
	}
	if expectation != nil {
	expectation.Replicas() // for sorting side-effect
	}

.

[tbg]

Why is this changing? Note that in #39484 I ran into problems because the sorting isn't stable but we're passing ReplicaID zero everywhere, just absorb that PR perhaps?

[tbg]

Oh wait the sorting is going away, nvm

[tbg]

Can't this just be the same underlying type now?

type ReplicaDescriptors []ReplicaDescriptor

[tbg]

I like this, good call. I thought about doing the same but didn't have the fast path on the radar.

In a world in which we have "permanent" learners we're going to have to fiddle with this once more, but let's buy into complexity when we truly need this

LGTM

[nvanbenschoten]

Reviewed 12 of 12 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @danhhz)

---

pkg/roachpb/metadata_replicas.go, line 170 at r1 (raw file):

// For some related mega-comments, see Replica.sendSnapshot.
func (d ReplicaDescriptors) Learners() []ReplicaDescriptor {
	learners := make([]ReplicaDescriptor, 0, len(d.wrapped))

Should we add a fast-path here as well? Most of the time we expect no LEARNERS, right? I guess that would be equivalent to not pre-allocating this slice:

var learners []ReplicaDesciptor

You might be worried about the potential for repeat allocations if we do that, in which case I'd still make the allocation lazy:

var learners []ReplicaDesciptor
for i := range d.wrapped {
    if d.wrapped[i].GetType() == ReplicaType_LEARNER {
        if learners == nil {
            learners = make([]ReplicaDescriptor, 0, len(d.wrapped)-i)
...

[danhhz]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @danhhz, @nvanbenschoten, and @tbg)

---

pkg/roachpb/metadata_replicas.go, line 52 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Can't this just be the same underlying type now?

type ReplicaDescriptors []ReplicaDescriptor

My understanding is there's no overhead to the struct as long as the only field is the wrapped slice and I like making users go through the abstraction. If it was just a type alias for the slice, even users external to the package could simply cast it

---

pkg/roachpb/metadata_replicas.go, line 170 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we add a fast-path here as well? Most of the time we expect no LEARNERS, right? I guess that would be equivalent to not pre-allocating this slice:

var learners []ReplicaDesciptor

You might be worried about the potential for repeat allocations if we do that, in which case I'd still make the allocation lazy:

var learners []ReplicaDesciptor
for i := range d.wrapped {
    if d.wrapped[i].GetType() == ReplicaType_LEARNER {
        if learners == nil {
            learners = make([]ReplicaDescriptor, 0, len(d.wrapped)-i)
...

Done

[danhhz]

TFTRs!

bors r=tbg,nvanbenschoten

[craig]

Build succeeded

• GitHub CI (Cockroach)