Merge pull request #74204 from nvanbenschoten/backport21.2-74108

release-21.2: kv: remove dependency on ticks from maybeDropMsgApp

pkg/kv/kvserver/replica.go

@@ -211,6 +211,9 @@ type Replica struct {
 	// The writes to this key happen in Replica.setStartKeyLocked.
 	startKey roachpb.RKey
 
+	// creationTime is the time that the Replica struct was initially constructed.
+	creationTime time.Time
+
 	store     *Store
 	abortSpan *abortspan.AbortSpan // Avoids anomalous reads after abort
 

pkg/kv/kvserver/replica_init.go

@@ -70,6 +70,7 @@ func newUnloadedReplica(
 	r := &Replica{
 		AmbientContext: store.cfg.AmbientCtx,
 		RangeID:        desc.RangeID,
+		creationTime:   timeutil.Now(),
 		store:          store,
 		abortSpan:      abortspan.New(desc.RangeID),
 		concMgr: concurrency.NewManager(concurrency.Config{

pkg/kv/kvserver/split_trigger_helper.go

@@ -13,37 +13,42 @@ package kvserver
 import (
 	"context"
 	"fmt"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"go.etcd.io/etcd/raft/v3/raftpb"
 )
 
-const maxDelaySplitTriggerTicks = 100
+const maxDelaySplitTriggerDur = 20 * time.Second
 
 type replicaMsgAppDropper Replica
 
-func (rd *replicaMsgAppDropper) Args() (initialized bool, ticks int) {
+func (rd *replicaMsgAppDropper) Args() (initialized bool, age time.Duration) {
 	r := (*Replica)(rd)
 	r.mu.RLock()
 	initialized = r.isInitializedRLocked()
-	ticks = r.mu.ticks
+	creationTime := r.creationTime
 	r.mu.RUnlock()
-	return initialized, ticks
+	age = timeutil.Since(creationTime)
+	return initialized, age
 }
 
-func (rd *replicaMsgAppDropper) ShouldDrop(startKey roachpb.RKey) (fmt.Stringer, bool) {
+func (rd *replicaMsgAppDropper) ShouldDrop(
+	ctx context.Context, startKey roachpb.RKey,
+) (fmt.Stringer, bool) {
 	lhsRepl := (*Replica)(rd).store.LookupReplica(startKey)
 	if lhsRepl == nil {
 		return nil, false
 	}
-	lhsRepl.store.replicaGCQueue.AddAsync(context.Background(), lhsRepl, replicaGCPriorityDefault)
+	lhsRepl.store.replicaGCQueue.AddAsync(ctx, lhsRepl, replicaGCPriorityDefault)
 	return lhsRepl, true
 }
 
 type msgAppDropper interface {
-	Args() (initialized bool, ticks int)
-	ShouldDrop(key roachpb.RKey) (fmt.Stringer, bool)
+	Args() (initialized bool, age time.Duration)
+	ShouldDrop(ctx context.Context, key roachpb.RKey) (fmt.Stringer, bool)
 }
 
 // maybeDropMsgApp returns true if the incoming Raft message should be dropped.
@@ -69,7 +74,7 @@ func maybeDropMsgApp(
 	// message via msg.Context. Check if this replica might be waiting for a
 	// split trigger. The first condition for that is not knowing the key
 	// bounds, i.e. not being initialized.
-	initialized, ticks := r.Args()
+	initialized, age := r.Args()
 
 	if initialized {
 		return false
@@ -130,24 +135,24 @@ func maybeDropMsgApp(
 
 	// NB: the caller is likely holding r.raftMu, but that's OK according to
 	// the lock order. We're not allowed to hold r.mu, but we don't.
-	lhsRepl, drop := r.ShouldDrop(startKey)
+	lhsRepl, drop := r.ShouldDrop(ctx, startKey)
 	if !drop {
 		return false
 	}
 
 	if verbose {
 		log.Infof(ctx, "start key is contained in replica %v", lhsRepl)
 	}
-	if ticks > maxDelaySplitTriggerTicks {
+	if age > maxDelaySplitTriggerDur {
 		// This is an escape hatch in case there are other scenarios (missed in
 		// the above analysis) in which a split trigger just isn't coming. If
 		// there are, the idea is that we notice this log message and improve
 		// the heuristics.
 		log.Warningf(
 			ctx,
 			"would have dropped incoming MsgApp to wait for split trigger, "+
-				"but allowing due to %d (>%d) ticks",
-			ticks, maxDelaySplitTriggerTicks)
+				"but allowing because uninitialized replica was created %s (>%s) ago",
+			age, maxDelaySplitTriggerDur)
 		return false
 	}
 	if verbose {

pkg/kv/kvserver/split_trigger_helper_test.go

@@ -14,6 +14,7 @@ import (
 	"context"
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
@@ -26,17 +27,19 @@ import (
 
 type testMsgAppDropper struct {
 	initialized bool
-	ticks       int
+	age         time.Duration
 	lhs         bool
 
 	startKey string // set by ShouldDrop
 }
 
-func (td *testMsgAppDropper) Args() (initialized bool, ticks int) {
-	return td.initialized, td.ticks
+func (td *testMsgAppDropper) Args() (initialized bool, age time.Duration) {
+	return td.initialized, td.age
 }
 
-func (td *testMsgAppDropper) ShouldDrop(startKey roachpb.RKey) (fmt.Stringer, bool) {
+func (td *testMsgAppDropper) ShouldDrop(
+	ctx context.Context, startKey roachpb.RKey,
+) (fmt.Stringer, bool) {
 	if len(startKey) == 0 {
 		panic("empty startKey")
 	}
@@ -56,9 +59,9 @@ func TestMaybeDropMsgApp(t *testing.T) {
 		// Drop message to wait for trigger.
 		{initialized: false, lhs: true}: true,
 		// Drop message to wait for trigger.
-		{initialized: false, lhs: true, ticks: maxDelaySplitTriggerTicks}: true,
+		{initialized: false, lhs: true, age: maxDelaySplitTriggerDur}: true,
 		// Escape hatch fires.
-		{initialized: false, lhs: true, ticks: maxDelaySplitTriggerTicks + 1}: false,
+		{initialized: false, lhs: true, age: maxDelaySplitTriggerDur + 1}: false,
 	}
 
 	msgHeartbeat := &raftpb.Message{