kvserver: improve suspect replica GC heuristics

[erikgrinaker]

The replica GC queue will normally check a replica against the canonical
range descriptor every 12 hours. Under some circumstances the replica
may be considered suspect, which causes it to be checked against the
canonical descriptor every second instead. However, these heuristics
were fairly limited and missed a couple of cases that could cause stale
replicas to linger.

This patch adds two conditions to the suspect replica heuristics:
followers that have lost contact with their leader (which in particular
handles non-voting replicas), and quiescent replicas that lose contact
with any other voters (which could cause false underreplication alerts).

Since this change is expected to increase suspect replica matches, the
ReplicaGCQueueSuspectCheckInterval duration between checking suspect
replica descriptors was also increased from 1 to 5 seconds, and the
replicaGCQueueTimerDuration interval between replica GCs was increased
from 50 to 100 ms.

The previous logic would take into account replica activity such as
store startup and lease proposals as the offset for timeouts, but this
did not appear to have any significant benefit over simply using the
last check time, so these have been removed and the timeouts given more
appropriate names. The previous logic also failed to enforce the check
interval for suspect replicas, and would always check them in a tight
50ms loop, this has been fixed as well.

Resolves #62075, resolves #60259.

Release note (bug fix): Improved garbage collection of stale replicas by
proactively checking certain replicas that have lost contact with other
voting replicas.

/cc @cockroachdb/kv

[cockroach-teamcity]

This change is 

[tbg]

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @erikgrinaker)

---

pkg/kv/kvserver/replica_gc_queue.go, line 191 at r1 (raw file):

		}

	// If the replica is a leader, check that is has a quorum. This handles e.g.

nit: it has a quorum

---

pkg/kv/kvserver/replica_gc_queue.go, line 231 at r1 (raw file):

	// Only queue for GC if the timeout interval has passed since both the last
	// lease was proposed and since the last check.

lastActivity is weird, you fill it with meaning here in this comment but maybe we can clarify what it indicates. As is, it's confusing. It is really just there to prevent us from considering replicas for replicaGC "prematurely", but is this buying us anything? It almost feels like this is maybe detritus from when we had preemptive snapshots (which were not part of the range desc, i.e. they would get removed whenever replicaGC looked at them). If we can remove it, that would be nice and from the looks of it we can.

[erikgrinaker]

Reviewed 3 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner and @tbg)

---

pkg/kv/kvserver/replica_gc_queue.go, line 231 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

lastActivity is weird, you fill it with meaning here in this comment but maybe we can clarify what it indicates. As is, it's confusing. It is really just there to prevent us from considering replicas for replicaGC "prematurely", but is this buying us anything? It almost feels like this is maybe detritus from when we had preemptive snapshots (which were not part of the range desc, i.e. they would get removed whenever replicaGC looked at them). If we can remove it, that would be nice and from the looks of it we can.

Yeah, I don't think it's needed, so I've removed it and renamed the timeout constants. It might serve some purpose in staggering the GC checks, but the replicas are likely to start stores and propose leases around the same time anyway, at least after cluster restarts.

[erikgrinaker]

bors r=tbg

[craig]

Build succeeded:

• GitHub CI (Cockroach)