From acdf42adb91c2ff03a87bb97d0ffa37d2baf9f9f Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Wed, 27 Jul 2022 11:41:46 +0000 Subject: [PATCH] roachpb: make range/replica descriptor fields non-nullable This patch makes all fields in range/replica descriptors non-nullable, fixing a long-standing TODO. Additionally, this fixes a set of bugs where code would use regular comparison operators (e.g. `==`) to compare replica descriptors, which with nullable pointer fields would compare the memory locations rather than the values. One practical consequence of this was that the DistSender would fail to use a new leaseholder with a non-`VOTER` type (e.g. `VOTER_INCOMING`), instead continuing to try other replicas before backing off. However, there are further issues surrounding this bug and it will be addressed separately in a way that is more amenable to backporting. The preparatory work for this was done in ea720e3e. Release note: None --- docs/generated/swagger/spec.json | 2 +- .../kvfollowerreadsccl/followerreads_test.go | 2 +- pkg/cli/debug_recover_loss_of_quorum_test.go | 3 +- pkg/kv/kvclient/kvcoord/dist_sender_test.go | 4 +- pkg/kv/kvclient/kvcoord/replica_slice_test.go | 6 +- pkg/kv/kvclient/kvcoord/split_test.go | 4 +- pkg/kv/kvclient/kvcoord/transport_test.go | 2 +- .../allocator/allocatorimpl/allocator_test.go | 31 ++-- .../kvserver/batcheval/cmd_end_transaction.go | 6 +- pkg/kv/kvserver/batcheval/cmd_lease_test.go | 12 +- pkg/kv/kvserver/client_split_burst_test.go | 2 +- .../kvserver/loqrecovery/recovery_env_test.go | 9 +- pkg/kv/kvserver/merge_queue.go | 6 +- pkg/kv/kvserver/raft_snapshot_queue.go | 2 +- pkg/kv/kvserver/replica_command.go | 65 +++----- pkg/kv/kvserver/replica_command_test.go | 7 +- pkg/kv/kvserver/replica_follower_read.go | 4 +- pkg/kv/kvserver/replica_gc_queue.go | 2 +- pkg/kv/kvserver/replica_learner_test.go | 6 +- pkg/kv/kvserver/replica_metrics_test.go | 24 +-- pkg/kv/kvserver/replica_proposal_buf_test.go | 2 +- pkg/kv/kvserver/replica_raft_overload_test.go | 6 +- pkg/kv/kvserver/replica_raftstorage.go | 4 +- pkg/kv/kvserver/replica_test.go | 3 +- pkg/kv/kvserver/replicate_queue.go | 4 +- pkg/kv/kvserver/replicate_queue_test.go | 4 +- pkg/kv/kvserver/store_rebalancer_test.go | 4 +- pkg/roachpb/data.go | 21 +-- pkg/roachpb/data_test.go | 20 +-- pkg/roachpb/metadata.go | 47 ++---- pkg/roachpb/metadata.proto | 16 +- pkg/roachpb/metadata_replicas.go | 61 ++------ pkg/roachpb/metadata_replicas_test.go | 143 ++++++++---------- pkg/sql/crdb_internal.go | 4 +- pkg/sql/gcjob/gc_job.go | 2 +- pkg/sql/unsplit_range_test.go | 6 +- pkg/sql/unsplit_test.go | 4 +- pkg/testutils/testcluster/testcluster.go | 6 +- 38 files changed, 212 insertions(+), 344 deletions(-) diff --git a/docs/generated/swagger/spec.json b/docs/generated/swagger/spec.json index dfa6adb3eb4d..a2bde3976710 100644 --- a/docs/generated/swagger/spec.json +++ b/docs/generated/swagger/spec.json @@ -1322,7 +1322,7 @@ "x-go-package": "github.com/cockroachdb/cockroach/pkg/server/serverpb" }, "ReplicaDescriptor": { - "description": "ReplicaDescriptor describes a replica location by node ID\n(corresponds to a host:port via lookup on gossip network) and store\nID (identifies the device).\nTODO(jeffreyxiao): All nullable fields in ReplicaDescriptor can be made\nnon-nullable if #38302 is guaranteed to be on all nodes (I.E. 20.1).", + "description": "ReplicaDescriptor describes a replica location by node ID\n(corresponds to a host:port via lookup on gossip network) and store\nID (identifies the device).", "type": "object", "properties": { "node_id": { diff --git a/pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go b/pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go index cf7e4f1f4efa..d208aada388d 100644 --- a/pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go +++ b/pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go @@ -660,7 +660,7 @@ func TestFollowerReadsWithStaleDescriptor(t *testing.T) { require.Equal(t, roachpb.StoreID(1), entry.Lease().Replica.StoreID) require.Equal(t, []roachpb.ReplicaDescriptor{ {NodeID: 1, StoreID: 1, ReplicaID: 1}, - {NodeID: 3, StoreID: 3, ReplicaID: 3, Type: roachpb.ReplicaTypeNonVoter()}, + {NodeID: 3, StoreID: 3, ReplicaID: 3, Type: roachpb.NON_VOTER}, }, entry.Desc().Replicas().Descriptors()) // Make a note of the follower reads metric on n3. We'll check that it was diff --git a/pkg/cli/debug_recover_loss_of_quorum_test.go b/pkg/cli/debug_recover_loss_of_quorum_test.go index d59314b6b669..0dc37b62fc4a 100644 --- a/pkg/cli/debug_recover_loss_of_quorum_test.go +++ b/pkg/cli/debug_recover_loss_of_quorum_test.go @@ -273,7 +273,6 @@ func createIntentOnRangeDescriptor( func TestJsonSerialization(t *testing.T) { defer leaktest.AfterTest(t)() - rt := roachpb.VOTER_INCOMING nr := loqrecoverypb.NodeReplicaInfo{ Replicas: []loqrecoverypb.ReplicaInfo{ { @@ -288,7 +287,7 @@ func TestJsonSerialization(t *testing.T) { NodeID: 1, StoreID: 2, ReplicaID: 3, - Type: &rt, + Type: roachpb.VOTER_INCOMING, }, }, NextReplicaID: 4, diff --git a/pkg/kv/kvclient/kvcoord/dist_sender_test.go b/pkg/kv/kvclient/kvcoord/dist_sender_test.go index 71ebf5ce4397..f2b072966f22 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender_test.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender_test.go @@ -585,7 +585,7 @@ func TestRetryOnNotLeaseHolderError(t *testing.T) { recognizedLeaseHolder := testUserRangeDescriptor3Replicas.Replicas().VoterDescriptors()[1] recognizedLeaseHolderIncoming := testUserRangeDescriptor3Replicas.Replicas().VoterDescriptors()[2] - recognizedLeaseHolderIncoming.Type = roachpb.ReplicaTypeVoterIncoming() + recognizedLeaseHolderIncoming.Type = roachpb.VOTER_INCOMING unrecognizedLeaseHolder := roachpb.ReplicaDescriptor{ NodeID: 99, StoreID: 999, @@ -712,7 +712,7 @@ func TestRetryOnNotLeaseHolderError(t *testing.T) { // initial range descriptor, not the one returned in the NLHE, i.e. // it won't have the non-nil type. expRetryReplica := *tc.expLeaseholder - expRetryReplica.Type = nil + expRetryReplica.Type = 0 require.Equal(t, expRetryReplica, retryReplica) } else { require.Nil(t, rng.Lease()) diff --git a/pkg/kv/kvclient/kvcoord/replica_slice_test.go b/pkg/kv/kvclient/kvcoord/replica_slice_test.go index 6cc425e28eba..c234edc69cc5 100644 --- a/pkg/kv/kvclient/kvcoord/replica_slice_test.go +++ b/pkg/kv/kvclient/kvcoord/replica_slice_test.go @@ -74,8 +74,7 @@ func TestNewReplicaSlice(t *testing.T) { require.Equal(t, 3, rs.Len()) // Check that learners are not included. - typLearner := roachpb.LEARNER - rd.InternalReplicas[2].Type = &typLearner + rd.InternalReplicas[2].Type = roachpb.LEARNER rs, err = NewReplicaSlice(ctx, ns, rd, nil, OnlyPotentialLeaseholders) require.NoError(t, err) require.Equal(t, 2, rs.Len()) @@ -84,8 +83,7 @@ func TestNewReplicaSlice(t *testing.T) { require.Equal(t, 2, rs.Len()) // Check that non-voters are included iff we ask for them to be. - typNonVoter := roachpb.NON_VOTER - rd.InternalReplicas[2].Type = &typNonVoter + rd.InternalReplicas[2].Type = roachpb.NON_VOTER rs, err = NewReplicaSlice(ctx, ns, rd, nil, AllExtantReplicas) require.NoError(t, err) require.Equal(t, 3, rs.Len()) diff --git a/pkg/kv/kvclient/kvcoord/split_test.go b/pkg/kv/kvclient/kvcoord/split_test.go index e68426720afa..573e8dcee4c0 100644 --- a/pkg/kv/kvclient/kvcoord/split_test.go +++ b/pkg/kv/kvclient/kvcoord/split_test.go @@ -290,7 +290,7 @@ func TestRangeSplitsStickyBit(t *testing.T) { if err != nil { t.Fatal(err) } - if desc.GetStickyBit().IsEmpty() { + if desc.StickyBit.IsEmpty() { t.Fatal("Sticky bit not set after splitting") } @@ -309,7 +309,7 @@ func TestRangeSplitsStickyBit(t *testing.T) { if err != nil { t.Fatal(err) } - if desc.GetStickyBit().IsEmpty() { + if desc.StickyBit.IsEmpty() { t.Fatal("Sticky bit not set after splitting") } } diff --git a/pkg/kv/kvclient/kvcoord/transport_test.go b/pkg/kv/kvclient/kvcoord/transport_test.go index 9318b72ac255..7c5e07a899ba 100644 --- a/pkg/kv/kvclient/kvcoord/transport_test.go +++ b/pkg/kv/kvclient/kvcoord/transport_test.go @@ -33,7 +33,7 @@ func TestTransportMoveToFront(t *testing.T) { rd2 := roachpb.ReplicaDescriptor{NodeID: 2, StoreID: 2, ReplicaID: 2} rd3 := roachpb.ReplicaDescriptor{NodeID: 3, StoreID: 3, ReplicaID: 3} rd3Incoming := roachpb.ReplicaDescriptor{NodeID: 3, StoreID: 3, ReplicaID: 3, - Type: roachpb.ReplicaTypeVoterIncoming()} + Type: roachpb.VOTER_INCOMING} gt := grpcTransport{replicas: []roachpb.ReplicaDescriptor{rd1, rd2, rd3}} verifyOrder := func(replicas []roachpb.ReplicaDescriptor) { diff --git a/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go b/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go index 49df25529c70..8699c700d5bb 100644 --- a/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go +++ b/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go @@ -5678,7 +5678,7 @@ func TestAllocatorComputeAction(t *testing.T) { StoreID: 4, NodeID: 4, ReplicaID: 4, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, }, }, @@ -5754,7 +5754,7 @@ func TestAllocatorComputeAction(t *testing.T) { StoreID: 4, NodeID: 4, ReplicaID: 4, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, }, }, @@ -6032,13 +6032,13 @@ func TestAllocatorComputeAction(t *testing.T) { StoreID: 4, NodeID: 4, ReplicaID: 4, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, { StoreID: 6, NodeID: 6, ReplicaID: 6, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, }, }, @@ -6090,7 +6090,7 @@ func TestAllocatorComputeAction(t *testing.T) { StoreID: 6, NodeID: 6, ReplicaID: 6, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, }, }, @@ -6113,13 +6113,13 @@ func TestAllocatorComputeAction(t *testing.T) { StoreID: 2, NodeID: 2, ReplicaID: 2, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, { StoreID: 6, NodeID: 6, ReplicaID: 6, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, }, }, @@ -6143,13 +6143,13 @@ func TestAllocatorComputeAction(t *testing.T) { StoreID: 2, NodeID: 2, ReplicaID: 2, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, { StoreID: 3, NodeID: 3, ReplicaID: 3, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, }, }, @@ -6646,19 +6646,19 @@ func TestAllocatorComputeActionDecommission(t *testing.T) { StoreID: 4, NodeID: 4, ReplicaID: 4, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, { StoreID: 6, NodeID: 6, ReplicaID: 6, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, { StoreID: 7, NodeID: 7, ReplicaID: 7, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, }, }, @@ -6683,13 +6683,13 @@ func TestAllocatorComputeActionDecommission(t *testing.T) { StoreID: 4, NodeID: 4, ReplicaID: 4, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, { StoreID: 6, NodeID: 6, ReplicaID: 6, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, }, }, @@ -6719,7 +6719,6 @@ func TestAllocatorRemoveLearner(t *testing.T) { defer log.Scope(t).Close(t) conf := roachpb.SpanConfig{NumReplicas: 3} - learnerType := roachpb.LEARNER rangeWithLearnerDesc := roachpb.RangeDescriptor{ InternalReplicas: []roachpb.ReplicaDescriptor{ { @@ -6731,7 +6730,7 @@ func TestAllocatorRemoveLearner(t *testing.T) { StoreID: 2, NodeID: 2, ReplicaID: 2, - Type: &learnerType, + Type: roachpb.LEARNER, }, }, } diff --git a/pkg/kv/kvserver/batcheval/cmd_end_transaction.go b/pkg/kv/kvserver/batcheval/cmd_end_transaction.go index 9ecd47faa890..5e3badf9ac67 100644 --- a/pkg/kv/kvserver/batcheval/cmd_end_transaction.go +++ b/pkg/kv/kvserver/batcheval/cmd_end_transaction.go @@ -707,11 +707,7 @@ func RunCommitTrigger( } if sbt := ct.GetStickyBitTrigger(); sbt != nil { newDesc := *rec.Desc() - if !sbt.StickyBit.IsEmpty() { - newDesc.StickyBit = &sbt.StickyBit - } else { - newDesc.StickyBit = nil - } + newDesc.StickyBit = sbt.StickyBit var res result.Result res.Replicated.State = &kvserverpb.ReplicaState{ Desc: &newDesc, diff --git a/pkg/kv/kvserver/batcheval/cmd_lease_test.go b/pkg/kv/kvserver/batcheval/cmd_lease_test.go index 41454001fbb2..fc14f711bb01 100644 --- a/pkg/kv/kvserver/batcheval/cmd_lease_test.go +++ b/pkg/kv/kvserver/batcheval/cmd_lease_test.go @@ -122,8 +122,8 @@ func TestLeaseCommandLearnerReplica(t *testing.T) { ctx := context.Background() const voterStoreID, learnerStoreID roachpb.StoreID = 1, 2 replicas := []roachpb.ReplicaDescriptor{ - {NodeID: 1, StoreID: voterStoreID, Type: roachpb.ReplicaTypeVoterFull(), ReplicaID: 1}, - {NodeID: 2, StoreID: learnerStoreID, Type: roachpb.ReplicaTypeLearner(), ReplicaID: 2}, + {NodeID: 1, StoreID: voterStoreID, Type: roachpb.VOTER_FULL, ReplicaID: 1}, + {NodeID: 2, StoreID: learnerStoreID, Type: roachpb.LEARNER, ReplicaID: 2}, } desc := roachpb.RangeDescriptor{} desc.SetReplicas(roachpb.MakeReplicaSet(replicas)) @@ -183,8 +183,8 @@ func TestLeaseTransferForwardsStartTime(t *testing.T) { defer batch.Close() replicas := []roachpb.ReplicaDescriptor{ - {NodeID: 1, StoreID: 1, Type: roachpb.ReplicaTypeVoterFull(), ReplicaID: 1}, - {NodeID: 2, StoreID: 2, Type: roachpb.ReplicaTypeVoterFull(), ReplicaID: 2}, + {NodeID: 1, StoreID: 1, Type: roachpb.VOTER_FULL, ReplicaID: 1}, + {NodeID: 2, StoreID: 2, Type: roachpb.VOTER_FULL, ReplicaID: 2}, } desc := roachpb.RangeDescriptor{} desc.SetReplicas(roachpb.MakeReplicaSet(replicas)) @@ -301,7 +301,7 @@ func TestCheckCanReceiveLease(t *testing.T) { t.Run(tc.leaseholderType.String(), func(t *testing.T) { repDesc := roachpb.ReplicaDescriptor{ ReplicaID: 1, - Type: &tc.leaseholderType, + Type: tc.leaseholderType, } rngDesc := roachpb.RangeDescriptor{ InternalReplicas: []roachpb.ReplicaDescriptor{repDesc}, @@ -309,7 +309,7 @@ func TestCheckCanReceiveLease(t *testing.T) { if tc.anotherReplicaType != none { anotherDesc := roachpb.ReplicaDescriptor{ ReplicaID: 2, - Type: &tc.anotherReplicaType, + Type: tc.anotherReplicaType, } rngDesc.InternalReplicas = append(rngDesc.InternalReplicas, anotherDesc) } diff --git a/pkg/kv/kvserver/client_split_burst_test.go b/pkg/kv/kvserver/client_split_burst_test.go index 74cab23db5d3..a02f528a0d3b 100644 --- a/pkg/kv/kvserver/client_split_burst_test.go +++ b/pkg/kv/kvserver/client_split_burst_test.go @@ -78,7 +78,7 @@ func setupSplitBurstTest(t *testing.T, delay time.Duration) *splitBurstTest { if args.Split == nil || delay == 0 { return 0, nil } - if args.Split.RightDesc.GetStickyBit() != magicStickyBit { + if args.Split.RightDesc.StickyBit != magicStickyBit { return 0, nil } select { diff --git a/pkg/kv/kvserver/loqrecovery/recovery_env_test.go b/pkg/kv/kvserver/loqrecovery/recovery_env_test.go index 242dc9f1e191..1e2e0d9601c9 100644 --- a/pkg/kv/kvserver/loqrecovery/recovery_env_test.go +++ b/pkg/kv/kvserver/loqrecovery/recovery_env_test.go @@ -105,10 +105,10 @@ type descriptorViewWrapper struct { // for the purpose of yaml representation and to avoid leaking test // specific code into production. type replicaDescriptorView struct { - NodeID roachpb.NodeID `yaml:"NodeID"` - StoreID roachpb.StoreID `yaml:"StoreID"` - ReplicaID roachpb.ReplicaID `yaml:"ReplicaID"` - ReplicaType *roachpb.ReplicaType `yaml:"ReplicaType,omitempty"` + NodeID roachpb.NodeID `yaml:"NodeID"` + StoreID roachpb.StoreID `yaml:"StoreID"` + ReplicaID roachpb.ReplicaID `yaml:"ReplicaID"` + ReplicaType roachpb.ReplicaType `yaml:"ReplicaType,omitempty"` } func (r replicaDescriptorView) asReplicaDescriptor() roachpb.ReplicaDescriptor { @@ -273,7 +273,6 @@ func buildReplicaDescriptorFromTestData( InternalReplicas: replicas, NextReplicaID: maxReplicaID + 1, Generation: replica.Generation, - StickyBit: nil, } lease := roachpb.Lease{ Start: clock.Now().Add(5*time.Minute.Nanoseconds(), 0).UnsafeToClockTimestamp(), diff --git a/pkg/kv/kvserver/merge_queue.go b/pkg/kv/kvserver/merge_queue.go index ee6a8e27fb3f..4c79cf5005aa 100644 --- a/pkg/kv/kvserver/merge_queue.go +++ b/pkg/kv/kvserver/merge_queue.go @@ -237,7 +237,7 @@ func (mq *mergeQueue) process( // Range was manually split and not expired, so skip merging. now := mq.store.Clock().NowAsClockTimestamp() - if now.ToTimestamp().Less(rhsDesc.GetStickyBit()) { + if now.ToTimestamp().Less(rhsDesc.StickyBit) { log.VEventf(ctx, 2, "skipping merge: ranges were manually split and sticky bit was not expired") // TODO(jeffreyxiao): Consider returning a purgatory error to avoid // repeatedly processing ranges that cannot be merged. @@ -309,7 +309,7 @@ func (mq *mergeQueue) process( // Defensive sanity check that the ranges involved only have either VOTER_FULL // and NON_VOTER replicas. for i := range leftRepls { - if typ := leftRepls[i].GetType(); !(typ == roachpb.VOTER_FULL || typ == roachpb.NON_VOTER) { + if typ := leftRepls[i].Type; !(typ == roachpb.VOTER_FULL || typ == roachpb.NON_VOTER) { return false, errors.AssertionFailedf( `cannot merge because lhs is either in a joint state or has learner replicas: %v`, @@ -370,7 +370,7 @@ func (mq *mergeQueue) process( rightRepls = rhsDesc.Replicas().Descriptors() } for i := range rightRepls { - if typ := rightRepls[i].GetType(); !(typ == roachpb.VOTER_FULL || typ == roachpb.NON_VOTER) { + if typ := rightRepls[i].Type; !(typ == roachpb.VOTER_FULL || typ == roachpb.NON_VOTER) { log.Infof(ctx, "RHS Type: %s", typ) return false, errors.AssertionFailedf( diff --git a/pkg/kv/kvserver/raft_snapshot_queue.go b/pkg/kv/kvserver/raft_snapshot_queue.go index b1a7668c270c..223747f98e8d 100644 --- a/pkg/kv/kvserver/raft_snapshot_queue.go +++ b/pkg/kv/kvserver/raft_snapshot_queue.go @@ -110,7 +110,7 @@ func (rq *raftSnapshotQueue) processRaftSnapshot( } snapType := kvserverpb.SnapshotRequest_VIA_SNAPSHOT_QUEUE - if typ := repDesc.GetType(); typ == roachpb.LEARNER || typ == roachpb.NON_VOTER { + if typ := repDesc.Type; typ == roachpb.LEARNER || typ == roachpb.NON_VOTER { if fn := repl.store.cfg.TestingKnobs.RaftSnapshotQueueSkipReplica; fn != nil && fn() { return false, nil } diff --git a/pkg/kv/kvserver/replica_command.go b/pkg/kv/kvserver/replica_command.go index 62de454680aa..46c91284d9b7 100644 --- a/pkg/kv/kvserver/replica_command.go +++ b/pkg/kv/kvserver/replica_command.go @@ -158,25 +158,11 @@ func prepareSplitDescs( // (updated) left hand side. See the comment on the field for an explanation // of why generations are useful. rightDesc.Generation = leftDesc.Generation + rightDesc.StickyBit = expiration - setStickyBit(rightDesc, expiration) return leftDesc, rightDesc } -func setStickyBit(desc *roachpb.RangeDescriptor, expiration hlc.Timestamp) { - // TODO(jeffreyxiao): Remove this check in 20.1. - // Note that the client API for splitting has expiration time as - // non-nullable, but the internal representation of a sticky bit is nullable - // for backwards compatibility. If expiration time is the zero timestamp, we - // must be sure not to set the sticky bit to the zero timestamp because the - // byte representation of setting the stickyBit to nil is different than - // setting it to hlc.Timestamp{}. This check ensures that CPuts would not - // fail on older versions. - if !expiration.IsEmpty() { - desc.StickyBit = &expiration - } -} - func splitTxnAttempt( ctx context.Context, store *Store, @@ -267,7 +253,7 @@ func splitTxnStickyUpdateAttempt( return err } newDesc := *desc - setStickyBit(&newDesc, expiration) + newDesc.StickyBit = expiration b := txn.NewBatch() descKey := keys.RangeDescriptorKey(desc.StartKey) @@ -283,7 +269,7 @@ func splitTxnStickyUpdateAttempt( Commit: true, InternalCommitTrigger: &roachpb.InternalCommitTrigger{ StickyBitTrigger: &roachpb.StickyBitTrigger{ - StickyBit: newDesc.GetStickyBit(), + StickyBit: newDesc.StickyBit, }, }, }) @@ -391,7 +377,7 @@ func (r *Replica) adminSplitWithDescriptor( log.Event(ctx, "range already split") // Even if the range is already split, we should still update the sticky // bit if it has a later expiration time. - if desc.GetStickyBit().Less(args.ExpirationTime) { + if desc.StickyBit.Less(args.ExpirationTime) { err := r.store.DB().Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { return splitTxnStickyUpdateAttempt(ctx, txn, desc, args.ExpirationTime) }) @@ -473,7 +459,7 @@ func (r *Replica) adminUnsplitWithDescriptor( // mixed version clusters that don't support StickyBit, all range descriptor // sticky bits are guaranteed to be nil, so we can skip checking the cluster // version. - if desc.GetStickyBit().IsEmpty() { + if desc.StickyBit.IsEmpty() { return reply, nil } @@ -485,11 +471,7 @@ func (r *Replica) adminUnsplitWithDescriptor( } newDesc := *desc - // Use nil instead of &zero until 20.1; this field is new in 19.2. We - // could use &zero here because the sticky bit will never be populated - // before the cluster version reaches 19.2 and the early return above - // already handles that case, but nothing is won in doing so. - newDesc.StickyBit = nil + newDesc.StickyBit = hlc.Timestamp{} descKey := keys.RangeDescriptorKey(newDesc.StartKey) b := txn.NewBatch() @@ -1206,8 +1188,7 @@ func (r *Replica) maybeTransferLeaseDuringLeaveJoint( if beingRemoved && v.ReplicaID == r.ReplicaID() { beingRemoved = false } - if voterIncomingTarget == (roachpb.ReplicaDescriptor{}) && - v.GetType() == roachpb.VOTER_INCOMING { + if voterIncomingTarget == (roachpb.ReplicaDescriptor{}) && v.Type == roachpb.VOTER_INCOMING { voterIncomingTarget = v } } @@ -1408,7 +1389,7 @@ func validateAdditionsPerStore( // good. continue } - switch t := replDesc.GetType(); t { + switch t := replDesc.Type; t { case roachpb.LEARNER: // Looks like we found a learner with the same store and node id. One of // the following is true: @@ -1453,7 +1434,7 @@ func validateRemovals(desc *roachpb.RangeDescriptor, chgsByStoreID changesByStor } // Ensure that the type of replica being removed is the same as the type // of replica present in the range descriptor. - switch t := replDesc.GetType(); t { + switch t := replDesc.Type; t { case roachpb.VOTER_FULL, roachpb.LEARNER: if chg.ChangeType != roachpb.REMOVE_VOTER { return errors.AssertionFailedf("type of replica being removed (%s) does not match"+ @@ -1494,7 +1475,7 @@ func validatePromotionsAndDemotions( // Thus, the store must not already have a replica. if found { return errors.AssertionFailedf("trying to add(%+v) to a store(%s) that already"+ - " has a replica(%s)", chgs[0], storeID, replDesc.GetType()) + " has a replica(%s)", chgs[0], storeID, replDesc.Type) } } case 2: @@ -1513,7 +1494,7 @@ func validatePromotionsAndDemotions( isDemotion := c1.ChangeType == roachpb.ADD_NON_VOTER && c2.ChangeType == roachpb.REMOVE_VOTER if !(isPromotion || isDemotion) { return errors.AssertionFailedf("trying to add-remove the same replica(%s):"+ - " %+v", replDesc.GetType(), chgs) + " %+v", replDesc.Type, chgs) } } else { // NB: validateOneReplicaPerNode has a stronger version of this check, @@ -1793,7 +1774,7 @@ func (r *Replica) initializeRaftLearners( return nil, errors.Errorf("programming error: replica %v not found in %v", target, desc) } - if rDesc.GetType() != replicaType { + if rDesc.Type != replicaType { return nil, errors.Errorf("programming error: cannot promote replica of type %s", rDesc.Type) } @@ -1888,7 +1869,7 @@ func (r *Replica) execReplicationChangesForVoters( for _, target := range voterRemovals { typ := internalChangeTypeRemoveLearner - if rDesc, ok := desc.GetReplicaDescriptor(target.StoreID); ok && rDesc.GetType() == roachpb.VOTER_FULL { + if rDesc, ok := desc.GetReplicaDescriptor(target.StoreID); ok && rDesc.Type == roachpb.VOTER_FULL { typ = internalChangeTypeDemoteVoterToLearner } iChgs = append(iChgs, internalReplicationChange{target: target, typ: typ}) @@ -1933,7 +1914,7 @@ func (r *Replica) tryRollbackRaftLearner( return } var removeChgType internalChangeType - switch repDesc.GetType() { + switch repDesc.Type { case roachpb.NON_VOTER: removeChgType = internalChangeTypeRemoveNonVoter case roachpb.LEARNER: @@ -1972,13 +1953,13 @@ func (r *Replica) tryRollbackRaftLearner( log.Infof( ctx, "failed to rollback %s %s, abandoning it for the replicate queue: %v", - repDesc.GetType(), + repDesc.Type, target, err, ) r.store.replicateQueue.MaybeAddAsync(ctx, r, r.store.Clock().NowAsClockTimestamp()) } else { - log.Infof(ctx, "rolled back %s %s in %s", repDesc.GetType(), target, rangeDesc) + log.Infof(ctx, "rolled back %s %s in %s", repDesc.Type, target, rangeDesc) } } @@ -2088,7 +2069,7 @@ func prepareChangeReplicasTrigger( if !ok { return nil, errors.Errorf("target %s not found", chg.target) } - prevTyp := rDesc.GetType() + prevTyp := rDesc.Type isRaftLearner := prevTyp == roachpb.LEARNER || prevTyp == roachpb.NON_VOTER if !useJoint || isRaftLearner { rDesc, _ = updatedDesc.RemoveReplica(chg.target.NodeID, chg.target.StoreID) @@ -2114,7 +2095,7 @@ func prepareChangeReplicasTrigger( // there's a demotion. This is just a sanity check. return nil, errors.AssertionFailedf("demotions require joint consensus") } - if prevTyp := rDesc.GetType(); prevTyp != roachpb.VOTER_FULL { + if prevTyp := rDesc.Type; prevTyp != roachpb.VOTER_FULL { return nil, errors.Errorf("cannot transition from %s to VOTER_DEMOTING_LEARNER", prevTyp) } rDesc, _, _ = updatedDesc.SetReplicaType(chg.target.NodeID, chg.target.StoreID, roachpb.VOTER_DEMOTING_LEARNER) @@ -2129,7 +2110,7 @@ func prepareChangeReplicasTrigger( // there's a demotion. This is just a sanity check. return nil, errors.Errorf("demotions require joint consensus") } - if prevTyp := rDesc.GetType(); prevTyp != roachpb.VOTER_FULL { + if prevTyp := rDesc.Type; prevTyp != roachpb.VOTER_FULL { return nil, errors.Errorf("cannot transition from %s to VOTER_DEMOTING_NON_VOTER", prevTyp) } rDesc, _, _ = updatedDesc.SetReplicaType(chg.target.NodeID, chg.target.StoreID, roachpb.VOTER_DEMOTING_NON_VOTER) @@ -2145,7 +2126,7 @@ func prepareChangeReplicasTrigger( // NB: the DeepCopy is needed or we'll skip over an entry every time we // call RemoveReplica below. for _, rDesc := range updatedDesc.Replicas().DeepCopy().Descriptors() { - switch rDesc.GetType() { + switch rDesc.Type { case roachpb.VOTER_INCOMING: updatedDesc.SetReplicaType(rDesc.NodeID, rDesc.StoreID, roachpb.VOTER_FULL) isJoint = true @@ -2455,7 +2436,7 @@ func recordRangeEventsInLog( logChange logChangeFn, ) error { for _, repDesc := range repDescs { - isNonVoter := repDesc.GetType() == roachpb.NON_VOTER + isNonVoter := repDesc.Type == roachpb.NON_VOTER var typ roachpb.ReplicaChangeType if added { typ = roachpb.ADD_VOTER @@ -3227,7 +3208,7 @@ func (r *Replica) relocateOne( NodeID: additionTarget.NodeID, StoreID: additionTarget.StoreID, ReplicaID: desc.NextReplicaID, - Type: roachpb.ReplicaTypeVoterFull(), + Type: roachpb.VOTER_FULL, }, ) // When we're relocating voting replicas, `additionTarget` is allowed to @@ -3250,7 +3231,7 @@ func (r *Replica) relocateOne( NodeID: additionTarget.NodeID, StoreID: additionTarget.StoreID, ReplicaID: desc.NextReplicaID, - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }, ) } diff --git a/pkg/kv/kvserver/replica_command_test.go b/pkg/kv/kvserver/replica_command_test.go index 696a6cf65cd7..7b6e9e772059 100644 --- a/pkg/kv/kvserver/replica_command_test.go +++ b/pkg/kv/kvserver/replica_command_test.go @@ -95,25 +95,24 @@ func TestRangeDescriptorUpdateProtoChangedAcrossVersions(t *testing.T) { func TestValidateReplicationChanges(t *testing.T) { defer leaktest.AfterTest(t)() - learnerType := roachpb.LEARNER twoVotersAndALearner := &roachpb.RangeDescriptor{ InternalReplicas: []roachpb.ReplicaDescriptor{ {NodeID: 1, StoreID: 1}, {NodeID: 3, StoreID: 3}, - {NodeID: 4, StoreID: 4, Type: &learnerType}, + {NodeID: 4, StoreID: 4, Type: roachpb.LEARNER}, }, } twoReplicasOnOneNode := &roachpb.RangeDescriptor{ InternalReplicas: []roachpb.ReplicaDescriptor{ {NodeID: 1, StoreID: 1}, {NodeID: 2, StoreID: 2}, - {NodeID: 1, StoreID: 3, Type: &learnerType}, + {NodeID: 1, StoreID: 3, Type: roachpb.LEARNER}, }, } oneVoterAndOneNonVoter := &roachpb.RangeDescriptor{ InternalReplicas: []roachpb.ReplicaDescriptor{ {NodeID: 1, StoreID: 1}, - {NodeID: 2, StoreID: 2, Type: roachpb.ReplicaTypeNonVoter()}, + {NodeID: 2, StoreID: 2, Type: roachpb.NON_VOTER}, }, } oneReplica := &roachpb.RangeDescriptor{ diff --git a/pkg/kv/kvserver/replica_follower_read.go b/pkg/kv/kvserver/replica_follower_read.go index 3edd73aa51b5..b5a952c7bd18 100644 --- a/pkg/kv/kvserver/replica_follower_read.go +++ b/pkg/kv/kvserver/replica_follower_read.go @@ -73,10 +73,10 @@ func (r *Replica) canServeFollowerReadRLocked(ctx context.Context, ba *roachpb.B return false } - switch typ := repDesc.GetType(); typ { + switch repDesc.Type { case roachpb.VOTER_FULL, roachpb.VOTER_INCOMING, roachpb.NON_VOTER: default: - log.Eventf(ctx, "%s replicas cannot serve follower reads", typ) + log.Eventf(ctx, "%s replicas cannot serve follower reads", repDesc.Type) return false } diff --git a/pkg/kv/kvserver/replica_gc_queue.go b/pkg/kv/kvserver/replica_gc_queue.go index a62b538050f3..9568f62a7a8e 100644 --- a/pkg/kv/kvserver/replica_gc_queue.go +++ b/pkg/kv/kvserver/replica_gc_queue.go @@ -143,7 +143,7 @@ func replicaIsSuspect(repl *Replica) bool { if !ok { return true } - if t := replDesc.GetType(); t != roachpb.VOTER_FULL && t != roachpb.NON_VOTER { + if t := replDesc.Type; t != roachpb.VOTER_FULL && t != roachpb.NON_VOTER { return true } diff --git a/pkg/kv/kvserver/replica_learner_test.go b/pkg/kv/kvserver/replica_learner_test.go index 1aebfb67d24b..c01123a4fa34 100644 --- a/pkg/kv/kvserver/replica_learner_test.go +++ b/pkg/kv/kvserver/replica_learner_test.go @@ -47,14 +47,14 @@ import ( ) func predIncoming(rDesc roachpb.ReplicaDescriptor) bool { - return rDesc.GetType() == roachpb.VOTER_INCOMING + return rDesc.Type == roachpb.VOTER_INCOMING } func predOutgoing(rDesc roachpb.ReplicaDescriptor) bool { - return rDesc.GetType() == roachpb.VOTER_OUTGOING + return rDesc.Type == roachpb.VOTER_OUTGOING } func predDemotingToLearner(rDesc roachpb.ReplicaDescriptor) bool { - return rDesc.GetType() == roachpb.VOTER_DEMOTING_LEARNER + return rDesc.Type == roachpb.VOTER_DEMOTING_LEARNER } type replicationTestKnobs struct { diff --git a/pkg/kv/kvserver/replica_metrics_test.go b/pkg/kv/kvserver/replica_metrics_test.go index ff2b84021789..045020410bdc 100644 --- a/pkg/kv/kvserver/replica_metrics_test.go +++ b/pkg/kv/kvserver/replica_metrics_test.go @@ -40,18 +40,18 @@ func TestCalcRangeCounterIsLiveMap(t *testing.T) { threeVotersAndSingleNonVoter := roachpb.NewRangeDescriptor(123, roachpb.RKeyMin, roachpb.RKeyMax, roachpb.MakeReplicaSet([]roachpb.ReplicaDescriptor{ - {NodeID: 10, StoreID: 11, ReplicaID: 12, Type: roachpb.ReplicaTypeVoterFull()}, - {NodeID: 100, StoreID: 110, ReplicaID: 120, Type: roachpb.ReplicaTypeVoterFull()}, - {NodeID: 1000, StoreID: 1100, ReplicaID: 1200, Type: roachpb.ReplicaTypeVoterFull()}, - {NodeID: 2000, StoreID: 2100, ReplicaID: 2200, Type: roachpb.ReplicaTypeNonVoter()}, + {NodeID: 10, StoreID: 11, ReplicaID: 12, Type: roachpb.VOTER_FULL}, + {NodeID: 100, StoreID: 110, ReplicaID: 120, Type: roachpb.VOTER_FULL}, + {NodeID: 1000, StoreID: 1100, ReplicaID: 1200, Type: roachpb.VOTER_FULL}, + {NodeID: 2000, StoreID: 2100, ReplicaID: 2200, Type: roachpb.NON_VOTER}, })) oneVoterAndThreeNonVoters := roachpb.NewRangeDescriptor(123, roachpb.RKeyMin, roachpb.RKeyMax, roachpb.MakeReplicaSet([]roachpb.ReplicaDescriptor{ - {NodeID: 10, StoreID: 11, ReplicaID: 12, Type: roachpb.ReplicaTypeVoterFull()}, - {NodeID: 100, StoreID: 110, ReplicaID: 120, Type: roachpb.ReplicaTypeNonVoter()}, - {NodeID: 1000, StoreID: 1100, ReplicaID: 1200, Type: roachpb.ReplicaTypeNonVoter()}, - {NodeID: 2000, StoreID: 2100, ReplicaID: 2200, Type: roachpb.ReplicaTypeNonVoter()}, + {NodeID: 10, StoreID: 11, ReplicaID: 12, Type: roachpb.VOTER_FULL}, + {NodeID: 100, StoreID: 110, ReplicaID: 120, Type: roachpb.NON_VOTER}, + {NodeID: 1000, StoreID: 1100, ReplicaID: 1200, Type: roachpb.NON_VOTER}, + {NodeID: 2000, StoreID: 2100, ReplicaID: 2200, Type: roachpb.NON_VOTER}, })) { @@ -144,10 +144,10 @@ func TestCalcRangeCounterLeaseHolder(t *testing.T) { rangeDesc := roachpb.NewRangeDescriptor(123, roachpb.RKeyMin, roachpb.RKeyMax, roachpb.MakeReplicaSet([]roachpb.ReplicaDescriptor{ - {NodeID: 1, StoreID: 10, ReplicaID: 100, Type: roachpb.ReplicaTypeVoterFull()}, - {NodeID: 2, StoreID: 20, ReplicaID: 200, Type: roachpb.ReplicaTypeNonVoter()}, - {NodeID: 3, StoreID: 30, ReplicaID: 300, Type: roachpb.ReplicaTypeVoterFull()}, - {NodeID: 4, StoreID: 40, ReplicaID: 400, Type: roachpb.ReplicaTypeVoterFull()}, + {NodeID: 1, StoreID: 10, ReplicaID: 100, Type: roachpb.VOTER_FULL}, + {NodeID: 2, StoreID: 20, ReplicaID: 200, Type: roachpb.NON_VOTER}, + {NodeID: 3, StoreID: 30, ReplicaID: 300, Type: roachpb.VOTER_FULL}, + {NodeID: 4, StoreID: 40, ReplicaID: 400, Type: roachpb.VOTER_FULL}, })) leaseStatus := kvserverpb.LeaseStatus{ diff --git a/pkg/kv/kvserver/replica_proposal_buf_test.go b/pkg/kv/kvserver/replica_proposal_buf_test.go index e620fb47231e..2dbb2d11e6d9 100644 --- a/pkg/kv/kvserver/replica_proposal_buf_test.go +++ b/pkg/kv/kvserver/replica_proposal_buf_test.go @@ -189,7 +189,7 @@ func (t *testProposer) leaderStatusRLocked( iAmTheLeader = leaderRep == t.getReplicaID() repDesc := roachpb.ReplicaDescriptor{ ReplicaID: leaderRep, - Type: &t.leaderReplicaType, + Type: t.leaderReplicaType, } if t.leaderReplicaInDescriptor { diff --git a/pkg/kv/kvserver/replica_raft_overload_test.go b/pkg/kv/kvserver/replica_raft_overload_test.go index 25ad84ea7e5e..bc11a5b69a13 100644 --- a/pkg/kv/kvserver/replica_raft_overload_test.go +++ b/pkg/kv/kvserver/replica_raft_overload_test.go @@ -72,9 +72,9 @@ func TestReplicaRaftOverload_computeExpendableOverloadedFollowers(t *testing.T) match[replicaID], err = strconv.ParseUint(matchS, 10, 64) require.NoError(t, err) } - typ := roachpb.ReplicaTypeVoterFull() + typ := roachpb.VOTER_FULL if arg.Key == "learners" { - typ = roachpb.ReplicaTypeNonVoter() + typ = roachpb.NON_VOTER } replDesc := roachpb.ReplicaDescriptor{ NodeID: roachpb.NodeID(id), @@ -112,7 +112,7 @@ func TestReplicaRaftOverload_computeExpendableOverloadedFollowers(t *testing.T) State: tracker.StateReplicate, Match: match[replDesc.ReplicaID], RecentActive: true, - IsLearner: replDesc.GetType() == roachpb.LEARNER || replDesc.GetType() == roachpb.NON_VOTER, + IsLearner: replDesc.Type == roachpb.LEARNER || replDesc.Type == roachpb.NON_VOTER, Inflights: tracker.NewInflights(1), // avoid NPE } m[uint64(replDesc.ReplicaID)] = pr diff --git a/pkg/kv/kvserver/replica_raftstorage.go b/pkg/kv/kvserver/replica_raftstorage.go index c2312ccab4c3..d7b6464798b2 100644 --- a/pkg/kv/kvserver/replica_raftstorage.go +++ b/pkg/kv/kvserver/replica_raftstorage.go @@ -821,7 +821,7 @@ func (r *Replica) applySnapshot( if isInitialSnap { r.store.metrics.RangeSnapshotsAppliedForInitialUpreplication.Inc(1) } else { - switch typ := desc.GetType(); typ { + switch desc.Type { // NB: A replica of type LEARNER can receive a non-initial snapshot (via // the snapshot queue) if we end up truncating the raft log before it // gets promoted to a voter. We count such snapshot applications as @@ -833,7 +833,7 @@ func (r *Replica) applySnapshot( case roachpb.NON_VOTER: r.store.metrics.RangeSnapshotsAppliedByNonVoters.Inc(1) default: - log.Fatalf(ctx, "unexpected replica type %s while applying snapshot", typ) + log.Fatalf(ctx, "unexpected replica type %s while applying snapshot", desc.Type) } } } diff --git a/pkg/kv/kvserver/replica_test.go b/pkg/kv/kvserver/replica_test.go index 7ec4672b0c28..58698df71dd3 100644 --- a/pkg/kv/kvserver/replica_test.go +++ b/pkg/kv/kvserver/replica_test.go @@ -13484,12 +13484,11 @@ func TestPrepareChangeReplicasTrigger(t *testing.T) { chgs := make([]internalReplicationChange, 0, len(typs)) rDescs := make([]roachpb.ReplicaDescriptor, 0, len(typs)) for i, typ := range typs { - typ := typ // local copy - we take addr below rDesc := roachpb.ReplicaDescriptor{ ReplicaID: roachpb.ReplicaID(i + 1), NodeID: roachpb.NodeID(100 * (1 + i)), StoreID: roachpb.StoreID(100 * (1 + i)), - Type: &(typ.ReplicaType), + Type: typ.ReplicaType, } if typ.ReplicaType != none { rDescs = append(rDescs, rDesc) diff --git a/pkg/kv/kvserver/replicate_queue.go b/pkg/kv/kvserver/replicate_queue.go index 7450595e8b99..97f6372dd883 100644 --- a/pkg/kv/kvserver/replicate_queue.go +++ b/pkg/kv/kvserver/replicate_queue.go @@ -860,7 +860,7 @@ func (rq *replicateQueue) addOrReplaceVoters( var ops []roachpb.ReplicationChange replDesc, found := desc.GetReplicaDescriptor(newVoter.StoreID) if found { - if replDesc.GetType() != roachpb.NON_VOTER { + if replDesc.Type != roachpb.NON_VOTER { return false, errors.AssertionFailedf("allocation target %s for a voter"+ " already has an unexpected replica: %s", newVoter, replDesc) } @@ -1481,7 +1481,7 @@ func replicationChangesForRebalance( switch rebalanceTargetType { case allocatorimpl.VoterTarget: // Check if the target being added already has a non-voting replica. - if found && rdesc.GetType() == roachpb.NON_VOTER { + if found && rdesc.Type == roachpb.NON_VOTER { // If the receiving store already has a non-voting replica, we *must* // execute a swap between that non-voting replica and the voting replica // we're trying to move to it. This swap is executed atomically via diff --git a/pkg/kv/kvserver/replicate_queue_test.go b/pkg/kv/kvserver/replicate_queue_test.go index b6ad078efee5..b2fd78760175 100644 --- a/pkg/kv/kvserver/replicate_queue_test.go +++ b/pkg/kv/kvserver/replicate_queue_test.go @@ -1152,7 +1152,7 @@ func TestReplicateQueueSwapVotersWithNonVoters(t *testing.T) { if !ok { return errors.Newf("no replica found on store %d", store) } - if typ := replDesc.GetType(); typ != roachpb.VOTER_FULL { + if typ := replDesc.Type; typ != roachpb.VOTER_FULL { return errors.Newf("replica on store %d does not match expectation;"+ " expected VOTER_FULL, got %s", typ) } @@ -1162,7 +1162,7 @@ func TestReplicateQueueSwapVotersWithNonVoters(t *testing.T) { if !ok { return errors.Newf("no replica found on store %d", store) } - if typ := replDesc.GetType(); typ != roachpb.NON_VOTER { + if typ := replDesc.Type; typ != roachpb.NON_VOTER { return errors.Newf("replica on store %d does not match expectation;"+ " expected NON_VOTER, got %s", typ) } diff --git a/pkg/kv/kvserver/store_rebalancer_test.go b/pkg/kv/kvserver/store_rebalancer_test.go index 32270340c0af..03028e5e304b 100644 --- a/pkg/kv/kvserver/store_rebalancer_test.go +++ b/pkg/kv/kvserver/store_rebalancer_test.go @@ -448,7 +448,7 @@ func loadRanges(rr *replicaRankings, s *Store, ranges []testRange) { NodeID: roachpb.NodeID(storeID), StoreID: storeID, ReplicaID: roachpb.ReplicaID(storeID), - Type: roachpb.ReplicaTypeVoterFull(), + Type: roachpb.VOTER_FULL, }) } repl.mu.state.Lease = &roachpb.Lease{ @@ -461,7 +461,7 @@ func loadRanges(rr *replicaRankings, s *Store, ranges []testRange) { NodeID: roachpb.NodeID(storeID), StoreID: storeID, ReplicaID: roachpb.ReplicaID(storeID), - Type: roachpb.ReplicaTypeNonVoter(), + Type: roachpb.NON_VOTER, }) } // TODO(a-robinson): The below three lines won't be needed once the old diff --git a/pkg/roachpb/data.go b/pkg/roachpb/data.go index f0dc9643ff0a..caaa2f048f73 100644 --- a/pkg/roachpb/data.go +++ b/pkg/roachpb/data.go @@ -1570,7 +1570,7 @@ func confChangeImpl( checkExists := func(in ReplicaDescriptor) error { for _, rDesc := range replicas { if rDesc.ReplicaID == in.ReplicaID { - if a, b := in.GetType(), rDesc.GetType(); a != b { + if in.Type != rDesc.Type { return errors.Errorf("have %s, but descriptor has %s", in, rDesc) } return nil @@ -1593,7 +1593,7 @@ func confChangeImpl( NodeID: uint64(rDesc.ReplicaID), }) - switch rDesc.GetType() { + switch rDesc.Type { case VOTER_OUTGOING: // If a voter is removed through joint consensus, it will // be turned into an outgoing voter first. @@ -1633,7 +1633,7 @@ func confChangeImpl( return nil, err } default: - return nil, errors.Errorf("can't remove replica in state %v", rDesc.GetType()) + return nil, errors.Errorf("can't remove replica in state %v", rDesc.Type) } } @@ -1646,7 +1646,7 @@ func confChangeImpl( } var changeType raftpb.ConfChangeType - switch rDesc.GetType() { + switch rDesc.Type { case VOTER_FULL: // We're adding a new voter. changeType = raftpb.ConfChangeAddNode @@ -1665,7 +1665,7 @@ func confChangeImpl( // A voter that is demoting was just removed and re-added in the // `removals` handler. We should not see it again here. // A voter that's outgoing similarly has no reason to show up here. - return nil, errors.Errorf("can't add replica in state %v", rDesc.GetType()) + return nil, errors.Errorf("can't add replica in state %v", rDesc.Type) } sl = append(sl, raftpb.ConfChangeSingle{ Type: changeType, @@ -1679,7 +1679,7 @@ func confChangeImpl( // descriptor already. var enteringJoint bool for _, rDesc := range replicas { - switch rDesc.GetType() { + switch rDesc.Type { case VOTER_INCOMING, VOTER_OUTGOING, VOTER_DEMOTING_LEARNER, VOTER_DEMOTING_NON_VOTER: enteringJoint = true default: @@ -1906,13 +1906,8 @@ func (l Lease) Equivalent(newL Lease) bool { // Ignore the ReplicaDescriptor's type. This shouldn't affect lease // equivalency because Raft state shouldn't be factored into the state of a // Replica's lease. We don't expect a leaseholder to ever become a LEARNER - // replica, but that also shouldn't prevent it from extending its lease. The - // code also avoids a potential bug where an unset ReplicaType and a set - // VOTER ReplicaType are considered distinct and non-equivalent. - // - // Change this line to the following when ReplicaType becomes non-nullable: - // l.Replica.Type, newL.Replica.Type = 0, 0 - l.Replica.Type, newL.Replica.Type = nil, nil + // replica, but that also shouldn't prevent it from extending its lease. + l.Replica.Type, newL.Replica.Type = 0, 0 // If both leases are epoch-based, we must dereference the epochs // and then set to nil. switch l.Type() { diff --git a/pkg/roachpb/data_test.go b/pkg/roachpb/data_test.go index ab3a960d7f9f..285489ba6d8e 100644 --- a/pkg/roachpb/data_test.go +++ b/pkg/roachpb/data_test.go @@ -995,8 +995,8 @@ func TestLeaseEquivalence(t *testing.T) { stasis2 := Lease{Replica: r1, Start: ts1, Epoch: 1, DeprecatedStartStasis: ts2.ToTimestamp().Clone()} r1Voter, r1Learner := r1, r1 - r1Voter.Type = ReplicaTypeVoterFull() - r1Learner.Type = ReplicaTypeLearner() + r1Voter.Type = VOTER_FULL + r1Learner.Type = LEARNER epoch1Voter := Lease{Replica: r1Voter, Start: ts1, Epoch: 1} epoch1Learner := Lease{Replica: r1Learner, Start: ts1, Epoch: 1} @@ -1815,14 +1815,10 @@ func TestUpdateObservedTimestamps(t *testing.T) { func TestChangeReplicasTrigger_String(t *testing.T) { defer leaktest.AfterTest(t)() - vi := VOTER_INCOMING - vo := VOTER_OUTGOING - vd := VOTER_DEMOTING_LEARNER - l := LEARNER - repl1 := ReplicaDescriptor{NodeID: 1, StoreID: 2, ReplicaID: 3, Type: &vi} - repl2 := ReplicaDescriptor{NodeID: 4, StoreID: 5, ReplicaID: 6, Type: &vo} - learner := ReplicaDescriptor{NodeID: 7, StoreID: 8, ReplicaID: 9, Type: &l} - repl3 := ReplicaDescriptor{NodeID: 10, StoreID: 11, ReplicaID: 12, Type: &vd} + repl1 := ReplicaDescriptor{NodeID: 1, StoreID: 2, ReplicaID: 3, Type: VOTER_INCOMING} + repl2 := ReplicaDescriptor{NodeID: 4, StoreID: 5, ReplicaID: 6, Type: VOTER_OUTGOING} + learner := ReplicaDescriptor{NodeID: 7, StoreID: 8, ReplicaID: 9, Type: LEARNER} + repl3 := ReplicaDescriptor{NodeID: 10, StoreID: 11, ReplicaID: 12, Type: VOTER_DEMOTING_LEARNER} crt := ChangeReplicasTrigger{ InternalAddedReplicas: []ReplicaDescriptor{repl1}, InternalRemovedReplicas: []ReplicaDescriptor{repl2, repl3}, @@ -1849,7 +1845,7 @@ func TestChangeReplicasTrigger_String(t *testing.T) { crt.InternalRemovedReplicas = nil crt.InternalAddedReplicas = nil - repl1.Type = ReplicaTypeVoterFull() + repl1.Type = VOTER_FULL crt.Desc.SetReplicas(MakeReplicaSet([]ReplicaDescriptor{repl1, learner})) act = crt.String() require.Empty(t, crt.Added()) @@ -1880,7 +1876,7 @@ func TestChangeReplicasTrigger_ConfChange(t *testing.T) { typ := alt[i].(ReplicaType) id := alt[i+1].(int) rDescs = append(rDescs, ReplicaDescriptor{ - Type: &typ, + Type: typ, NodeID: NodeID(3 * id), StoreID: StoreID(2 * id), ReplicaID: ReplicaID(id), diff --git a/pkg/roachpb/metadata.go b/pkg/roachpb/metadata.go index a00714e006d1..b1c75e2ace84 100644 --- a/pkg/roachpb/metadata.go +++ b/pkg/roachpb/metadata.go @@ -18,7 +18,6 @@ import ( "strings" "github.com/cockroachdb/cockroach/pkg/util" - "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/humanizeutil" "github.com/cockroachdb/errors" "github.com/cockroachdb/redact" @@ -258,13 +257,8 @@ func (r *RangeDescriptor) SetReplicaType( for i := range r.InternalReplicas { desc := &r.InternalReplicas[i] if desc.StoreID == storeID && desc.NodeID == nodeID { - prevTyp := desc.GetType() - if typ != VOTER_FULL { - desc.Type = &typ - } else { - // For 19.1 compatibility. - desc.Type = nil - } + prevTyp := desc.Type + desc.Type = typ return *desc, prevTyp, true } } @@ -276,16 +270,11 @@ func (r *RangeDescriptor) SetReplicaType( func (r *RangeDescriptor) AddReplica( nodeID NodeID, storeID StoreID, typ ReplicaType, ) ReplicaDescriptor { - var typPtr *ReplicaType - // For 19.1 compatibility, use nil instead of VOTER_FULL. - if typ != VOTER_FULL { - typPtr = &typ - } toAdd := ReplicaDescriptor{ NodeID: nodeID, StoreID: storeID, ReplicaID: r.NextReplicaID, - Type: typPtr, + Type: typ, } rs := r.Replicas() rs.AddReplica(toAdd) @@ -335,14 +324,6 @@ func (r *RangeDescriptor) IncrementGeneration() { r.Generation++ } -// GetStickyBit returns the sticky bit of this RangeDescriptor. -func (r *RangeDescriptor) GetStickyBit() hlc.Timestamp { - if r.StickyBit == nil { - return hlc.Timestamp{} - } - return *r.StickyBit -} - // Validate performs some basic validation of the contents of a range descriptor. func (r *RangeDescriptor) Validate() error { if r.NextReplicaID == 0 { @@ -397,8 +378,8 @@ func (r RangeDescriptor) SafeFormat(w redact.SafePrinter, _ rune) { w.SafeString("") } w.Printf(", next=%d, gen=%d", r.NextReplicaID, r.Generation) - if s := r.GetStickyBit(); !s.IsEmpty() { - w.Printf(", sticky=%s", s) + if !r.StickyBit.IsEmpty() { + w.Printf(", sticky=%s", r.StickyBit) } w.SafeString("]") } @@ -430,8 +411,8 @@ func (r ReplicaDescriptor) SafeFormat(w redact.SafePrinter, _ rune) { } else { w.Print(r.ReplicaID) } - if typ := r.GetType(); typ != VOTER_FULL { - w.Print(typ) + if r.Type != VOTER_FULL { + w.Print(r.Type) } } @@ -449,14 +430,6 @@ func (r ReplicaDescriptor) Validate() error { return nil } -// GetType returns the type of this ReplicaDescriptor. -func (r ReplicaDescriptor) GetType() ReplicaType { - if r.Type == nil { - return VOTER_FULL - } - return *r.Type -} - // SafeValue implements the redact.SafeValue interface. func (r ReplicaType) SafeValue() {} @@ -476,7 +449,7 @@ func (r ReplicaSet) GetReplicaDescriptorByID(id ReplicaID) (repDesc ReplicaDescr // Can be used as a filter for // ReplicaDescriptors.Filter(ReplicaDescriptor.IsVoterOldConfig). func (r ReplicaDescriptor) IsVoterOldConfig() bool { - switch r.GetType() { + switch r.Type { case VOTER_FULL, VOTER_OUTGOING, VOTER_DEMOTING_NON_VOTER, VOTER_DEMOTING_LEARNER: return true default: @@ -489,7 +462,7 @@ func (r ReplicaDescriptor) IsVoterOldConfig() bool { // Can be used as a filter for // ReplicaDescriptors.Filter(ReplicaDescriptor.IsVoterOldConfig). func (r ReplicaDescriptor) IsVoterNewConfig() bool { - switch r.GetType() { + switch r.Type { case VOTER_FULL, VOTER_INCOMING: return true default: @@ -501,7 +474,7 @@ func (r ReplicaDescriptor) IsVoterNewConfig() bool { // config (pre-reconfiguration) or the incoming config. Can be used as a filter // for ReplicaDescriptors.Filter(ReplicaDescriptor.IsVoterOldConfig). func (r ReplicaDescriptor) IsAnyVoter() bool { - switch r.GetType() { + switch r.Type { case VOTER_FULL, VOTER_INCOMING, VOTER_OUTGOING, VOTER_DEMOTING_NON_VOTER, VOTER_DEMOTING_LEARNER: return true default: diff --git a/pkg/roachpb/metadata.proto b/pkg/roachpb/metadata.proto index 72ca92f89a13..b77833f61c57 100644 --- a/pkg/roachpb/metadata.proto +++ b/pkg/roachpb/metadata.proto @@ -126,8 +126,6 @@ enum ReplicaType { // ReplicaDescriptor describes a replica location by node ID // (corresponds to a host:port via lookup on gossip network) and store // ID (identifies the device). -// TODO(jeffreyxiao): All nullable fields in ReplicaDescriptor can be made -// non-nullable if #38302 is guaranteed to be on all nodes (I.E. 20.1). message ReplicaDescriptor { option (gogoproto.goproto_stringer) = false; option (gogoproto.equal) = true; @@ -147,9 +145,9 @@ message ReplicaDescriptor { (gogoproto.customname) = "ReplicaID", (gogoproto.casttype) = "ReplicaID", (gogoproto.moretags) = "yaml:\"ReplicaID\""]; - // Type indicates which raft activities a replica participates in. A nil type - // is equivalent to VOTER. - optional ReplicaType type = 4 [(gogoproto.moretags) = "yaml:\"ReplicaType,omitempty\""]; + // Type indicates which raft activities a replica participates in. + optional ReplicaType type = 4 [(gogoproto.nullable) = false, + (gogoproto.moretags) = "yaml:\"ReplicaType,omitempty\""]; } // ReplicaIdent uniquely identifies a specific replica. @@ -171,9 +169,6 @@ message ReplicaIdent { // (instead of re-marshaling the proto), but unfortunately we also need the // Equal() method to work. Also note that we configure our protos to not // maintain unrecognized fields. -// -// TODO(jeffreyxiao): All nullable fields in RangeDescriptor can be made -// non-nullable if #38302 is guaranteed to be on all nodes (I.E. 20.1). message RangeDescriptor { option (gogoproto.goproto_stringer) = false; // We implement the Equal method by hand so that it can ignore deprecated @@ -271,8 +266,7 @@ message RangeDescriptor { // that the unsplit operation is a different operation from the merge // operation. Unsplit only unsets sticky_bit. It is represented by a // timestamp that indicates when it expires. After the expiration time has - // passed, the split is eligible for automatic merging. A nil sticky bit is - // equivalent to hlc.Timestamp{}. + // passed, the split is eligible for automatic merging. // // The reason the sticky_bit exists is because when the merge queue is // enabled and a manual split happens, the split ranges would immediately be @@ -280,7 +274,7 @@ message RangeDescriptor { // attempted to execute ALTER TABLE/INDEX ... SPLIT AT ... when the merge // queue is enabled. With sticky_bit, users can manually split ranges without // disabling the merge queue. - optional util.hlc.Timestamp sticky_bit = 7; + optional util.hlc.Timestamp sticky_bit = 7 [(gogoproto.nullable) = false]; reserved 8; } diff --git a/pkg/roachpb/metadata_replicas.go b/pkg/roachpb/metadata_replicas.go index 903a58cf0b2f..aa531a78abbb 100644 --- a/pkg/roachpb/metadata_replicas.go +++ b/pkg/roachpb/metadata_replicas.go @@ -18,48 +18,6 @@ import ( "go.etcd.io/etcd/raft/v3/raftpb" ) -// ReplicaTypeVoterFull returns a VOTER_FULL pointer suitable for use in a -// nullable proto field. -func ReplicaTypeVoterFull() *ReplicaType { - t := VOTER_FULL - return &t -} - -// ReplicaTypeVoterIncoming returns a VOTER_INCOMING pointer suitable -// for use in a nullable proto field. -func ReplicaTypeVoterIncoming() *ReplicaType { - t := VOTER_INCOMING - return &t -} - -// ReplicaTypeVoterOutgoing returns a VOTER_OUTGOING pointer suitable -// for use in a nullable proto field. -func ReplicaTypeVoterOutgoing() *ReplicaType { - t := VOTER_OUTGOING - return &t -} - -// ReplicaTypeVoterDemotingLearner returns a VOTER_DEMOTING_LEARNER pointer -// suitable for use in a nullable proto field. -func ReplicaTypeVoterDemotingLearner() *ReplicaType { - t := VOTER_DEMOTING_LEARNER - return &t -} - -// ReplicaTypeLearner returns a LEARNER pointer suitable for use in -// a nullable proto field. -func ReplicaTypeLearner() *ReplicaType { - t := LEARNER - return &t -} - -// ReplicaTypeNonVoter returns a NON_VOTER pointer suitable for use in -// a nullable proto field. -func ReplicaTypeNonVoter() *ReplicaType { - t := NON_VOTER - return &t -} - // ReplicaSet is a set of replicas, usually the nodes/stores on which // replicas of a range are stored. type ReplicaSet struct { @@ -97,7 +55,7 @@ func (d ReplicaSet) Descriptors() []ReplicaDescriptor { } func predVoterFull(rDesc ReplicaDescriptor) bool { - switch rDesc.GetType() { + switch rDesc.Type { case VOTER_FULL: return true default: @@ -106,7 +64,7 @@ func predVoterFull(rDesc ReplicaDescriptor) bool { } func predVoterFullOrIncoming(rDesc ReplicaDescriptor) bool { - switch rDesc.GetType() { + switch rDesc.Type { case VOTER_FULL, VOTER_INCOMING: return true default: @@ -115,7 +73,7 @@ func predVoterFullOrIncoming(rDesc ReplicaDescriptor) bool { } func predVoterIncoming(rDesc ReplicaDescriptor) bool { - switch rDesc.GetType() { + switch rDesc.Type { case VOTER_INCOMING: return true default: @@ -124,11 +82,11 @@ func predVoterIncoming(rDesc ReplicaDescriptor) bool { } func predLearner(rDesc ReplicaDescriptor) bool { - return rDesc.GetType() == LEARNER + return rDesc.Type == LEARNER } func predNonVoter(rDesc ReplicaDescriptor) bool { - return rDesc.GetType() == NON_VOTER + return rDesc.Type == NON_VOTER } func predVoterOrNonVoter(rDesc ReplicaDescriptor) bool { @@ -360,13 +318,13 @@ func (d *ReplicaSet) RemoveReplica(nodeID NodeID, storeID StoreID) (ReplicaDescr // an atomic replication change. func (d ReplicaSet) InAtomicReplicationChange() bool { for _, rDesc := range d.wrapped { - switch rDesc.GetType() { + switch rDesc.Type { case VOTER_INCOMING, VOTER_OUTGOING, VOTER_DEMOTING_LEARNER, VOTER_DEMOTING_NON_VOTER: return true case VOTER_FULL, LEARNER, NON_VOTER: default: - panic(fmt.Sprintf("unknown replica type %d", rDesc.GetType())) + panic(fmt.Sprintf("unknown replica type %d", rDesc.Type)) } } return false @@ -381,8 +339,7 @@ func (d ReplicaSet) ConfState() raftpb.ConfState { // category. for _, rep := range d.wrapped { id := uint64(rep.ReplicaID) - typ := rep.GetType() - switch typ { + switch rep.Type { case VOTER_FULL: cs.Voters = append(cs.Voters, id) if joint { @@ -400,7 +357,7 @@ func (d ReplicaSet) ConfState() raftpb.ConfState { case NON_VOTER: cs.Learners = append(cs.Learners, id) default: - panic(fmt.Sprintf("unknown ReplicaType %d", typ)) + panic(fmt.Sprintf("unknown ReplicaType %d", rep.Type)) } } return cs diff --git a/pkg/roachpb/metadata_replicas_test.go b/pkg/roachpb/metadata_replicas_test.go index a9286c341416..3261e8e7b537 100644 --- a/pkg/roachpb/metadata_replicas_test.go +++ b/pkg/roachpb/metadata_replicas_test.go @@ -28,7 +28,7 @@ import ( "go.etcd.io/etcd/raft/v3/tracker" ) -func rd(typ *ReplicaType, id uint64) ReplicaDescriptor { +func rd(typ ReplicaType, id uint64) ReplicaDescriptor { return ReplicaDescriptor{ Type: typ, NodeID: NodeID(100 * id), @@ -37,34 +37,24 @@ func rd(typ *ReplicaType, id uint64) ReplicaDescriptor { } } -var vn = (*ReplicaType)(nil) // should be treated like VoterFull -var v = ReplicaTypeVoterFull() -var vi = ReplicaTypeVoterIncoming() -var vo = ReplicaTypeVoterOutgoing() -var vd = ReplicaTypeVoterDemotingLearner() -var l = ReplicaTypeLearner() - func TestVotersLearnersAll(t *testing.T) { tests := [][]ReplicaDescriptor{ {}, - {rd(v, 1)}, - {rd(vn, 1)}, - {rd(l, 1)}, - {rd(v, 1), rd(l, 2), rd(v, 3)}, - {rd(vn, 1), rd(l, 2), rd(v, 3)}, - {rd(l, 1), rd(v, 2), rd(l, 3)}, - {rd(l, 1), rd(vn, 2), rd(l, 3)}, - {rd(vi, 1)}, - {rd(vo, 1)}, - {rd(l, 1), rd(vo, 2), rd(vi, 3), rd(vi, 4)}, + {rd(VOTER_FULL, 1)}, + {rd(LEARNER, 1)}, + {rd(VOTER_FULL, 1), rd(LEARNER, 2), rd(VOTER_FULL, 3)}, + {rd(LEARNER, 1), rd(VOTER_FULL, 2), rd(LEARNER, 3)}, + {rd(VOTER_INCOMING, 1)}, + {rd(VOTER_OUTGOING, 1)}, + {rd(LEARNER, 1), rd(VOTER_OUTGOING, 2), rd(VOTER_INCOMING, 3), rd(VOTER_INCOMING, 4)}, } for _, test := range tests { t.Run("", func(t *testing.T) { r := MakeReplicaSet(test) seen := map[ReplicaDescriptor]struct{}{} for _, voter := range r.VoterDescriptors() { - typ := voter.GetType() + typ := voter.Type switch typ { case VOTER_FULL, VOTER_INCOMING: seen[voter] = struct{}{} @@ -74,18 +64,17 @@ func TestVotersLearnersAll(t *testing.T) { } for _, learner := range r.LearnerDescriptors() { seen[learner] = struct{}{} - assert.Equal(t, LEARNER, learner.GetType()) + assert.Equal(t, LEARNER, learner.Type) } all := r.Descriptors() // Make sure that VOTER_OUTGOING is the only type that is skipped both // by LearnerDescriptors() and VoterDescriptors() for _, rd := range all { - typ := rd.GetType() if _, seen := seen[rd]; !seen { - assert.Equal(t, VOTER_OUTGOING, typ) + assert.Equal(t, VOTER_OUTGOING, rd.Type) } else { - assert.NotEqual(t, VOTER_OUTGOING, typ) + assert.NotEqual(t, VOTER_OUTGOING, rd.Type) } } assert.Equal(t, len(test), len(all)) @@ -119,7 +108,7 @@ func TestReplicaDescriptorsRemove(t *testing.T) { {NodeID: 1, StoreID: 1}, {NodeID: 2, StoreID: 2}, {NodeID: 3, StoreID: 3}, - {NodeID: 4, StoreID: 4, Type: ReplicaTypeLearner()}, + {NodeID: 4, StoreID: 4, Type: LEARNER}, }, remove: ReplicationTarget{NodeID: 2, StoreID: 2}, expected: true, @@ -138,10 +127,10 @@ func TestReplicaDescriptorsRemove(t *testing.T) { assert.Equal(t, lenBefore, len(r.Descriptors()), "testcase %d", i) } for _, voter := range r.VoterDescriptors() { - assert.Equal(t, VOTER_FULL, voter.GetType(), "testcase %d", i) + assert.Equal(t, VOTER_FULL, voter.Type, "testcase %d", i) } for _, learner := range r.LearnerDescriptors() { - assert.Equal(t, LEARNER, learner.GetType(), "testcase %d", i) + assert.Equal(t, LEARNER, learner.Type, "testcase %d", i) } } } @@ -152,16 +141,11 @@ func TestReplicaDescriptorsConfState(t *testing.T) { out string }{ { - []ReplicaDescriptor{rd(v, 1)}, - "Voters:[1] VotersOutgoing:[] Learners:[] LearnersNext:[] AutoLeave:false", - }, - // Make sure nil is treated like VoterFull. - { - []ReplicaDescriptor{rd(vn, 1)}, + []ReplicaDescriptor{rd(VOTER_FULL, 1)}, "Voters:[1] VotersOutgoing:[] Learners:[] LearnersNext:[] AutoLeave:false", }, { - []ReplicaDescriptor{rd(l, 1), rd(vn, 2)}, + []ReplicaDescriptor{rd(LEARNER, 1), rd(VOTER_FULL, 2)}, "Voters:[2] VotersOutgoing:[] Learners:[1] LearnersNext:[] AutoLeave:false", }, // First joint case. We're adding n3 (via atomic replication changes), so the outgoing @@ -169,33 +153,33 @@ func TestReplicaDescriptorsConfState(t *testing.T) { // Note that we could simplify this config so that it's not joint, but raft expects // the config exactly as described by the descriptor so we don't try. { - []ReplicaDescriptor{rd(l, 1), rd(v, 2), rd(vi, 3)}, + []ReplicaDescriptor{rd(LEARNER, 1), rd(VOTER_FULL, 2), rd(VOTER_INCOMING, 3)}, "Voters:[2 3] VotersOutgoing:[2] Learners:[1] LearnersNext:[] AutoLeave:false", }, // More complex joint change: a replica swap, switching out n4 for n3 from the initial // set of voters n2, n4 (plus learner n1 before and after). { - []ReplicaDescriptor{rd(l, 1), rd(v, 2), rd(vi, 3), rd(vo, 4)}, + []ReplicaDescriptor{rd(LEARNER, 1), rd(VOTER_FULL, 2), rd(VOTER_INCOMING, 3), rd(VOTER_OUTGOING, 4)}, "Voters:[2 3] VotersOutgoing:[2 4] Learners:[1] LearnersNext:[] AutoLeave:false", }, // Upreplicating from n1,n2 to n1,n2,n3,n4. { - []ReplicaDescriptor{rd(v, 1), rd(v, 2), rd(vi, 3), rd(vi, 4)}, + []ReplicaDescriptor{rd(VOTER_FULL, 1), rd(VOTER_FULL, 2), rd(VOTER_INCOMING, 3), rd(VOTER_INCOMING, 4)}, "Voters:[1 2 3 4] VotersOutgoing:[1 2] Learners:[] LearnersNext:[] AutoLeave:false", }, // Downreplicating from n1,n2,n3,n4 to n1,n2. { - []ReplicaDescriptor{rd(v, 1), rd(v, 2), rd(vo, 3), rd(vo, 4)}, + []ReplicaDescriptor{rd(VOTER_FULL, 1), rd(VOTER_FULL, 2), rd(VOTER_OUTGOING, 3), rd(VOTER_OUTGOING, 4)}, "Voters:[1 2] VotersOutgoing:[1 2 3 4] Learners:[] LearnersNext:[] AutoLeave:false", }, // Completely switching to a new set of replicas: n1,n2 to n4,n5. Throw a learner in for fun. { - []ReplicaDescriptor{rd(vo, 1), rd(vo, 2), rd(vi, 3), rd(vi, 4), rd(l, 5)}, + []ReplicaDescriptor{rd(VOTER_OUTGOING, 1), rd(VOTER_OUTGOING, 2), rd(VOTER_INCOMING, 3), rd(VOTER_INCOMING, 4), rd(LEARNER, 5)}, "Voters:[3 4] VotersOutgoing:[1 2] Learners:[5] LearnersNext:[] AutoLeave:false", }, // Throw in a voter demotion. The demoting voter should be treated as Outgoing and LearnersNext. { - []ReplicaDescriptor{rd(vo, 1), rd(vd, 2), rd(vi, 3), rd(vi, 4), rd(l, 5)}, + []ReplicaDescriptor{rd(VOTER_OUTGOING, 1), rd(VOTER_DEMOTING_LEARNER, 2), rd(VOTER_INCOMING, 3), rd(VOTER_INCOMING, 4), rd(LEARNER, 5)}, "Voters:[3 4] VotersOutgoing:[1 2] Learners:[5] LearnersNext:[2] AutoLeave:false", }, } @@ -222,84 +206,84 @@ func TestReplicaDescriptorsCanMakeProgress(t *testing.T) { exp bool }{ // One out of one voter dead. - {[]descWithLiveness{{false, rd(v, 1)}}, false}, + {[]descWithLiveness{{false, rd(VOTER_FULL, 1)}}, false}, // Three out of three voters dead. {[]descWithLiveness{ - {false, rd(v, 1)}, - {false, rd(v, 2)}, - {false, rd(v, 3)}, + {false, rd(VOTER_FULL, 1)}, + {false, rd(VOTER_FULL, 2)}, + {false, rd(VOTER_FULL, 3)}, }, false}, // Two out of three voters dead. {[]descWithLiveness{ - {false, rd(v, 1)}, - {true, rd(v, 2)}, - {false, rd(v, 3)}, + {false, rd(VOTER_FULL, 1)}, + {true, rd(VOTER_FULL, 2)}, + {false, rd(VOTER_FULL, 3)}, }, false}, // Two out of three voters alive. {[]descWithLiveness{ - {true, rd(v, 1)}, - {false, rd(v, 2)}, - {true, rd(v, 3)}, + {true, rd(VOTER_FULL, 1)}, + {false, rd(VOTER_FULL, 2)}, + {true, rd(VOTER_FULL, 3)}, }, true}, // Two out of three voters alive, but one is an incoming voter. The outgoing // group doesn't have quorum. {[]descWithLiveness{ - {true, rd(v, 1)}, - {false, rd(v, 2)}, - {true, rd(vi, 3)}, + {true, rd(VOTER_FULL, 1)}, + {false, rd(VOTER_FULL, 2)}, + {true, rd(VOTER_INCOMING, 3)}, }, false}, // Two out of three voters alive, but one is an outgoing voter. The incoming // group doesn't have quorum. {[]descWithLiveness{ - {true, rd(v, 1)}, - {false, rd(v, 2)}, - {true, rd(vd, 3)}, + {true, rd(VOTER_FULL, 1)}, + {false, rd(VOTER_FULL, 2)}, + {true, rd(VOTER_DEMOTING_LEARNER, 3)}, }, false}, // Two out of three voters dead, and they're all incoming voters. (This // can't happen in practice because it means there were zero voters prior // to the conf change, but still this result is correct, similar to others // below). {[]descWithLiveness{ - {false, rd(vi, 1)}, - {false, rd(vi, 2)}, - {true, rd(vi, 3)}, + {false, rd(VOTER_INCOMING, 2)}, + {false, rd(VOTER_INCOMING, 1)}, + {true, rd(VOTER_INCOMING, 3)}, }, false}, // Two out of three voters dead, and two are outgoing, one incoming. {[]descWithLiveness{ - {false, rd(vi, 1)}, - {false, rd(vo, 2)}, - {true, rd(vo, 3)}, + {false, rd(VOTER_INCOMING, 1)}, + {false, rd(VOTER_OUTGOING, 2)}, + {true, rd(VOTER_OUTGOING, 3)}, }, false}, // 1 and 3 are alive, but that's not a quorum for (1 3)&&(2 3) which is // the config here. {[]descWithLiveness{ - {true, rd(vi, 1)}, - {false, rd(vo, 2)}, - {true, rd(v, 3)}, + {true, rd(VOTER_INCOMING, 1)}, + {false, rd(VOTER_OUTGOING, 2)}, + {true, rd(VOTER_FULL, 3)}, }, false}, // Same as above, but all three alive. {[]descWithLiveness{ - {true, rd(vi, 1)}, - {true, rd(vo, 2)}, - {true, rd(v, 3)}, + {true, rd(VOTER_INCOMING, 1)}, + {true, rd(VOTER_OUTGOING, 2)}, + {true, rd(VOTER_FULL, 3)}, }, true}, // Same, but there are a few learners that should not matter. {[]descWithLiveness{ - {true, rd(vi, 1)}, - {true, rd(vo, 2)}, - {true, rd(v, 3)}, - {false, rd(l, 4)}, - {false, rd(l, 5)}, - {false, rd(l, 6)}, - {false, rd(l, 7)}, + {true, rd(VOTER_INCOMING, 1)}, + {true, rd(VOTER_OUTGOING, 2)}, + {true, rd(VOTER_FULL, 3)}, + {false, rd(LEARNER, 4)}, + {false, rd(LEARNER, 5)}, + {false, rd(LEARNER, 6)}, + {false, rd(LEARNER, 7)}, }, true}, // Non-joint case that should be live unless the learner is somehow taken // into account. {[]descWithLiveness{ - {true, rd(v, 1)}, - {true, rd(v, 2)}, - {false, rd(v, 4)}, - {false, rd(l, 4)}, + {true, rd(VOTER_FULL, 1)}, + {true, rd(VOTER_FULL, 2)}, + {false, rd(VOTER_FULL, 4)}, + {false, rd(LEARNER, 4)}, }, true}, } { t.Run("", func(t *testing.T) { @@ -341,8 +325,7 @@ func TestReplicaDescriptorsCanMakeProgressRandom(t *testing.T) { livenessBits := rand.Int31() for i := range rds { rds[i].ReplicaID = ReplicaID(i + 1) - typ := ReplicaType(rand.Intn(len(ReplicaType_name))) - rds[i].Type = &typ + rds[i].Type = ReplicaType(rand.Intn(len(ReplicaType_name))) liveness[i] = (livenessBits >> i & 1) == 0 } diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 44fd5154ff7d..6a659099db1b 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -3445,8 +3445,8 @@ CREATE TABLE crdb_internal.ranges_no_leases ( ) splitEnforcedUntil := tree.DNull - if !desc.GetStickyBit().IsEmpty() { - splitEnforcedUntil = eval.TimestampToInexactDTimestamp(*desc.StickyBit) + if !desc.StickyBit.IsEmpty() { + splitEnforcedUntil = eval.TimestampToInexactDTimestamp(desc.StickyBit) } return tree.Datums{ diff --git a/pkg/sql/gcjob/gc_job.go b/pkg/sql/gcjob/gc_job.go index 4d049cf3c8a3..496df4b4fb91 100644 --- a/pkg/sql/gcjob/gc_job.go +++ b/pkg/sql/gcjob/gc_job.go @@ -111,7 +111,7 @@ func unsplitRangesInSpan(ctx context.Context, kvDB *kv.DB, span roachpb.Span) er continue } - if !desc.GetStickyBit().IsEmpty() { + if !desc.StickyBit.IsEmpty() { // Swallow "key is not the start of a range" errors because it would mean // that the sticky bit was removed and merged concurrently. DROP TABLE // should not fail because of this. diff --git a/pkg/sql/unsplit_range_test.go b/pkg/sql/unsplit_range_test.go index e380bf635ab9..8442f2cbee86 100644 --- a/pkg/sql/unsplit_range_test.go +++ b/pkg/sql/unsplit_range_test.go @@ -68,7 +68,7 @@ func hasManuallySplitRangesInSpan( if err := r.ValueProto(&desc); err != nil { t.Fatal(err) } - if !desc.GetStickyBit().IsEmpty() { + if !desc.StickyBit.IsEmpty() { return true } } @@ -94,7 +94,7 @@ func hasManuallySplitRangesOnIndex( if err != nil { continue } - if indexID == descpb.IndexID(foundIndexID) && !desc.GetStickyBit().IsEmpty() { + if indexID == descpb.IndexID(foundIndexID) && !desc.StickyBit.IsEmpty() { return true } } @@ -147,7 +147,7 @@ func rangeIsManuallySplit( if err := r.ValueProto(&desc); err != nil { t.Fatal(err) } - if bytes.Equal(desc.StartKey.AsRawKey(), startKey) && !desc.GetStickyBit().IsEmpty() { + if bytes.Equal(desc.StartKey.AsRawKey(), startKey) && !desc.StickyBit.IsEmpty() { return true } } diff --git a/pkg/sql/unsplit_test.go b/pkg/sql/unsplit_test.go index 5c109e8c7191..c78207e491ae 100644 --- a/pkg/sql/unsplit_test.go +++ b/pkg/sql/unsplit_test.go @@ -207,8 +207,8 @@ func TestUnsplitAt(t *testing.T) { if err != nil { t.Fatal(err) } - if !rng.GetStickyBit().IsEmpty() { - t.Fatalf("%s: expected range sticky bit to be hlc.MinTimestamp, got %s", tt.unsplitStmt, rng.GetStickyBit()) + if !rng.StickyBit.IsEmpty() { + t.Fatalf("%s: expected range sticky bit to be hlc.MinTimestamp, got %s", tt.unsplitStmt, rng.StickyBit) } } if err := rows.Err(); err != nil { diff --git a/pkg/testutils/testcluster/testcluster.go b/pkg/testutils/testcluster/testcluster.go index 3bd9c24331f2..2719e6d17594 100644 --- a/pkg/testutils/testcluster/testcluster.go +++ b/pkg/testutils/testcluster/testcluster.go @@ -887,7 +887,7 @@ func (tc *TestCluster) waitForNewReplicas( desc := repl.Desc() if replDesc, ok := desc.GetReplicaDescriptor(target.StoreID); !ok { return errors.Errorf("target store %d not yet in range descriptor %v", target.StoreID, desc) - } else if waitForVoter && replDesc.GetType() != roachpb.VOTER_FULL { + } else if waitForVoter && replDesc.Type != roachpb.VOTER_FULL { return errors.Errorf("target store %d not yet voter in range descriptor %v", target.StoreID, desc) } } @@ -983,10 +983,10 @@ func (tc *TestCluster) SwapVoterWithNonVoterOrFatal( require.NoError(t, err) replDesc, ok := afterDesc.GetReplicaDescriptor(voterTarget.StoreID) require.True(t, ok) - require.Equal(t, roachpb.NON_VOTER, replDesc.GetType()) + require.Equal(t, roachpb.NON_VOTER, replDesc.Type) replDesc, ok = afterDesc.GetReplicaDescriptor(nonVoterTarget.StoreID) require.True(t, ok) - require.Equal(t, roachpb.VOTER_FULL, replDesc.GetType()) + require.Equal(t, roachpb.VOTER_FULL, replDesc.Type) return afterDesc }