roachpb: Change RangeDescriptor.StickyBit and ReplicaDescriptor.Type to nullable

[jeffrey-xiao]

On a mixed version cluster, only having #38302 does not guarantee backwards compatibility for non-nullable fields in RangeDescriptor. Suppose the replicas of a range are of mixed version and the current
leaseholder of the range is on newest version. If the leaseholder splits that range and writes to the non-nullable fields in RangeDescriptor and the leaseholder transfers to a node on an older version without #38302, then all CPuts would fail on that node.

We must guarantee that #38302 is on all nodes before making the fields in RangeDescriptor non-nullable.

Fixes #36983.
Fixes #38471.

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 12 of 13 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @jeffrey-xiao, and @nvanbenschoten)

---

pkg/storage/replica_command.go, line 188 at r1 (raw file):

	// it to hlc.Timestamp{}. This check ensures that CPuts would not fail on
	// older versions.
	stickyBitEnabled := r.store.ClusterSettings().Version.IsActive(cluster.VersionStickyBit)

We will only ever set the sticky bit on an AdminSplitRequest to a value that's not its zero value if this is already true, right? In that case, I think we should replace this with a check that the expiration time in the request is not the zero value. The reason for this is that nodes can learn that a cluster version is active at different times and we want to avoid the case where one node starts supplying an expiration value in its AdminSplitRequest, expecting this to be respected, while the node that evaluates the request (which hasn't yet learned about the new cluster version) is still dropping the values on the floor.

[jeffrey-xiao]

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

---

pkg/storage/replica_command.go, line 188 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We will only ever set the sticky bit on an AdminSplitRequest to a value that's not its zero value if this is already true, right? In that case, I think we should replace this with a check that the expiration time in the request is not the zero value. The reason for this is that nodes can learn that a cluster version is active at different times and we want to avoid the case where one node starts supplying an expiration value in its AdminSplitRequest, expecting this to be respected, while the node that evaluates the request (which hasn't yet learned about the new cluster version) is still dropping the values on the floor.

Thanks for the catch!

[nvanbenschoten]

Reviewed 1 of 1 files at r2, 7 of 8 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @jeffrey-xiao, and @nvanbenschoten)

---

pkg/roachpb/metadata_replicas.go, line 49 at r3 (raw file):

	// Note that the wrapped replicas are sorted first by type.
	for i := range d.wrapped {
		if d.wrapped[i].Type != nil && *d.wrapped[i].Type == ReplicaType_LEARNER {

Do you think it would be worth creating and using a method like:

func (rt *ReplicaType) Deref() ReplicaType {
    if rt == nil {
        return ReplicaType_VOTER
    }
    return *rt
}

It would belong in pkg/roachpb/metadata.go if we did.

---

pkg/roachpb/metadata_replicas_test.go, line 26 at r3 (raw file):

	tests := [][]ReplicaDescriptor{
		{},
		{{Type: newReplicaType(ReplicaType_VOTER)}},

nit: I'd pull these two allocs up and use them in each test case. Something like:

func TestVotersLearnersAll(t *testing.T) {
    voter := newReplicaType(ReplicaType_VOTER)
    learner := newReplicaType(ReplicaType_LEARNER)
    ...

[jeffrey-xiao]

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

---

pkg/roachpb/metadata_replicas.go, line 49 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do you think it would be worth creating and using a method like:

func (rt *ReplicaType) Deref() ReplicaType {
    if rt == nil {
        return ReplicaType_VOTER
    }
    return *rt
}

It would belong in pkg/roachpb/metadata.go if we did.

Done. I've added a method to ReplicaDescriptor similar to what we do with Generation for RangeDescriptor.

[nvanbenschoten]

thanks for turning this around so quickly.

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

[jeffrey-xiao]

Was inspired by the GetType change, so I also added GetStickyBit.

[nvanbenschoten]

Still

Reviewed 7 of 14 files at r5, 1 of 1 files at r6, 8 of 8 files at r7.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @danhhz)

[jeffrey-xiao]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[danhhz]

ReplicaDescriptor.Type stuff lgtm. Thanks for the fix!

[jeffrey-xiao]

Seems to be a flake in TeamCity?
bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[jeffrey-xiao]

Flake: #38458, #38468

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)