Merge #84211

84211: kv: raft closed timestamp is no longer nullable r=nvanbenschoten a=surahman The Raft protocol closed timestamp is no longer nullable. This change does not require a per-range migration because other migrations (namely, AddRaftAppliedIndexTermMigration) have already run across all ranges since v21.1 when this field was initially added. Those migrations ensured that all ranges now have a non-nil RaftClosedTimestamp. Closes #84104. Jira issue: [CRDB-17462](https://cockroachlabs.atlassian.net/browse/CRDB-17462) Co-authored-by: Saad Ur Rahman <[email protected]>
cockroachdb · Aug 1, 2022 · b83e83b · b83e83b
2 parents 7dae9c4 + de6bc15
commit b83e83b
Show file tree

Hide file tree

Showing 6 changed files with 22 additions and 28 deletions.
diff --git a/pkg/kv/kvserver/below_raft_protos_test.go b/pkg/kv/kvserver/below_raft_protos_test.go
@@ -77,8 +77,8 @@ var belowRaftGoldenProtos = map[reflect.Type]fixture{
 		populatedConstructor: func(r *rand.Rand) protoutil.Message {
 			return enginepb.NewPopulatedRangeAppliedState(r, false)
 		},
-		emptySum:     615555020845646359,
-		populatedSum: 4888917721712214316,
+		emptySum:     10160370728048384381,
+		populatedSum: 13858955692092952193,
 	},
 	reflect.TypeOf(&raftpb.HardState{}): {
 		populatedConstructor: func(r *rand.Rand) protoutil.Message {

diff --git a/pkg/kv/kvserver/raft_log_truncator_test.go b/pkg/kv/kvserver/raft_log_truncator_test.go
@@ -24,6 +24,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/util/encoding"
+	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
@@ -184,7 +185,7 @@ func (r *replicaTruncatorTest) writeRaftAppliedIndex(
 	t *testing.T, eng storage.Engine, raftAppliedIndex uint64, flush bool,
 ) {
 	require.NoError(t, r.stateLoader.SetRangeAppliedState(context.Background(), eng,
-		raftAppliedIndex, 0, 0, &enginepb.MVCCStats{}, nil, nil))
+		raftAppliedIndex, 0, 0, &enginepb.MVCCStats{}, hlc.Timestamp{}, nil))
 	// Flush to make it satisfy the contract of OnlyReadGuaranteedDurable in
 	// Pebble.
 	if flush {

diff --git a/pkg/kv/kvserver/replica_application_state_machine.go b/pkg/kv/kvserver/replica_application_state_machine.go
@@ -1041,7 +1041,7 @@ func (b *replicaAppBatch) addAppliedStateKeyToBatch(ctx context.Context) error {
 	loader := &b.r.raftMu.stateLoader
 	return loader.SetRangeAppliedState(
 		ctx, b.batch, b.state.RaftAppliedIndex, b.state.LeaseAppliedIndex, b.state.RaftAppliedIndexTerm,
-		b.state.Stats, &b.state.RaftClosedTimestamp, &b.asAlloc,
+		b.state.Stats, b.state.RaftClosedTimestamp, &b.asAlloc,
 	)
 }
 

diff --git a/pkg/kv/kvserver/stateloader/stateloader.go b/pkg/kv/kvserver/stateloader/stateloader.go
@@ -82,9 +82,7 @@ func (rsl StateLoader) Load(
 	s.LeaseAppliedIndex = as.LeaseAppliedIndex
 	ms := as.RangeStats.ToStats()
 	s.Stats = &ms
-	if as.RaftClosedTimestamp != nil {
-		s.RaftClosedTimestamp = *as.RaftClosedTimestamp
-	}
+	s.RaftClosedTimestamp = as.RaftClosedTimestamp
 
 	// The truncated state should not be optional (i.e. the pointer is
 	// pointless), but it is and the migration is not worth it.
@@ -134,9 +132,16 @@ func (rsl StateLoader) Save(
 			return enginepb.MVCCStats{}, err
 		}
 	}
-	rai, lai, rait, ct := state.RaftAppliedIndex, state.LeaseAppliedIndex, state.RaftAppliedIndexTerm,
-		&state.RaftClosedTimestamp
-	if err := rsl.SetRangeAppliedState(ctx, readWriter, rai, lai, rait, ms, ct, nil); err != nil {
+	if err := rsl.SetRangeAppliedState(
+		ctx,
+		readWriter,
+		state.RaftAppliedIndex,
+		state.LeaseAppliedIndex,
+		state.RaftAppliedIndexTerm,
+		ms,
+		state.RaftClosedTimestamp,
+		nil,
+	); err != nil {
 		return enginepb.MVCCStats{}, err
 	}
 	return *ms, nil
@@ -188,17 +193,12 @@ func (rsl StateLoader) LoadMVCCStats(
 // The applied indices and the stats used to be stored separately in different
 // keys. We now deem those keys to be "legacy" because they have been replaced
 // by the range applied state key.
-//
-// TODO(andrei): raftClosedTimestamp is a pointer to avoid an allocation when
-// putting it in RangeAppliedState. Once RangeAppliedState.RaftClosedTimestamp
-// is made non-nullable (see comments on the field), this argument should be
-// taken by value.
 func (rsl StateLoader) SetRangeAppliedState(
 	ctx context.Context,
 	readWriter storage.ReadWriter,
 	appliedIndex, leaseAppliedIndex, appliedIndexTerm uint64,
 	newMS *enginepb.MVCCStats,
-	raftClosedTimestamp *hlc.Timestamp,
+	raftClosedTimestamp hlc.Timestamp,
 	asAlloc *enginepb.RangeAppliedState, // optional
 ) error {
 	if asAlloc == nil {
@@ -209,11 +209,9 @@ func (rsl StateLoader) SetRangeAppliedState(
 		RaftAppliedIndex:     appliedIndex,
 		LeaseAppliedIndex:    leaseAppliedIndex,
 		RangeStats:           newMS.ToPersistentStats(),
+		RaftClosedTimestamp:  raftClosedTimestamp,
 		RaftAppliedIndexTerm: appliedIndexTerm,
 	}
-	if raftClosedTimestamp != nil && !raftClosedTimestamp.IsEmpty() {
-		as.RaftClosedTimestamp = raftClosedTimestamp
-	}
 	// The RangeAppliedStateKey is not included in stats. This is also reflected
 	// in ComputeStatsForRange.
 	ms := (*enginepb.MVCCStats)(nil)
@@ -239,7 +237,7 @@ func (rsl StateLoader) SetMVCCStats(
 
 // SetClosedTimestamp overwrites the closed timestamp.
 func (rsl StateLoader) SetClosedTimestamp(
-	ctx context.Context, readWriter storage.ReadWriter, closedTS *hlc.Timestamp,
+	ctx context.Context, readWriter storage.ReadWriter, closedTS hlc.Timestamp,
 ) error {
 	as, err := rsl.LoadRangeAppliedState(ctx, readWriter)
 	if err != nil {

diff --git a/pkg/kv/kvserver/store_split.go b/pkg/kv/kvserver/store_split.go
@@ -22,7 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/errors"
-	raft "go.etcd.io/etcd/raft/v3"
+	"go.etcd.io/etcd/raft/v3"
 	"go.etcd.io/etcd/raft/v3/raftpb"
 )
 
@@ -146,7 +146,7 @@ func splitPreApply(
 		initClosedTS = &hlc.Timestamp{}
 	}
 	initClosedTS.Forward(r.GetCurrentClosedTimestamp(ctx))
-	if err := rsl.SetClosedTimestamp(ctx, readWriter, initClosedTS); err != nil {
+	if err := rsl.SetClosedTimestamp(ctx, readWriter, *initClosedTS); err != nil {
 		log.Fatalf(ctx, "%s", err)
 	}
 }

diff --git a/pkg/storage/enginepb/mvcc3.proto b/pkg/storage/enginepb/mvcc3.proto
@@ -258,12 +258,7 @@ message RangeAppliedState {
   // commands that can still apply are writing at higher timestamps.
   // Non-leaseholder replicas are free to serve "follower reads" at or below
   // this timestamp.
-  //
-  // TODO(andrei): Make this field not-nullable in 21.2, once all the ranges
-  // have a closed timestamp applied to their state (this might need a
-  // migration). In 21.1 we cannot write empty timestamp to disk because that
-  // looks like an inconsistency to the consistency-checker.
-  util.hlc.Timestamp raft_closed_timestamp = 4;
+  util.hlc.Timestamp raft_closed_timestamp = 4 [(gogoproto.nullable) = false];
 
   // raft_applied_index_term is the term corresponding to raft_applied_index.
   // The serialized proto will not contain this field until code starts