storage: Maintain a separate set of unquiesced replicas

[bdarnell]

This means that idle replicas no longer have a per-tick CPU cost,
which is one of the bottlenecks limiting the amount of data we can
handle per store.

Fixes #17609

Release note (performance improvement): Reduced CPU overhead of idle
ranges

The first five commits are from #24920; that PR should be merged and tested in isolation first.

[cockroach-teamcity]

This change is 

[petermattis]

Reviewed 2 of 2 files at r1, 4 of 4 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Review status: 2 of 5 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

pkg/storage/replica.go, line 592 at r4 (raw file):

	anotherOwnsLease := r.leaseStatus(*r.mu.state.Lease, r.store.Clock().Now(), r.mu.minLeaseProposedTS).State ==
		LeaseState_VALID &&
		!r.mu.state.Lease.OwnedBy(r.store.StoreID())

We've historically had subtle errors in logic like this. Perhaps this code should be restructured for testability. For example, the logic could be placed in a shouldCampaignOnWakeLocked function which takes a lease and other information and returns a boolean.

---

pkg/storage/replica.go, line 3914 at r8 (raw file):

// needsTick returns true if tick() should be called on this replica.
func (r *Replica) needsTick() bool {

Is this method being used anymore?

---

pkg/storage/store.go, line 511 at r8 (raw file):

	}

	// The uniquesed subset of mu.replicas.

s/uniquesed/unquiesced/g

---

pkg/storage/store.go, line 512 at r8 (raw file):

	// The uniquesed subset of mu.replicas.
	unquiescedReplicas syncutil.IntMap

syncutil.IntMap might not be a good idea here as it is optimized for read-mostly workloads.

---

Comments from Reviewable

[tbg]

mod @petermattis' comments and waiting until the precursor has merged.

---

Reviewed 2 of 4 files at r2, 2 of 2 files at r6, 1 of 1 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

pkg/storage/store.go, line 2308 at r8 (raw file):

		return errors.Errorf("%s: replica already exists", repl)
	}
	s.unquiescedReplicas.Store(int64(repl.RangeID), unsafe.Pointer(repl))

LoadOrStore for symmetry (and an assertion)?

---

Comments from Reviewable

[bdarnell]

Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

pkg/storage/replica.go, line 3914 at r8 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Is this method being used anymore?

No. Removed.

---

pkg/storage/store.go, line 511 at r8 (raw file):

Previously, petermattis (Peter Mattis) wrote…

s/uniquesed/unquiesced/g

Done.

---

pkg/storage/store.go, line 512 at r8 (raw file):

Previously, petermattis (Peter Mattis) wrote…

syncutil.IntMap might not be a good idea here as it is optimized for read-mostly workloads.

OK, changed to a regular map with a mutex (a new mutex, since lock ordering gets tricky if we add it to Store.mu)

---

pkg/storage/store.go, line 2308 at r8 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

LoadOrStore for symmetry (and an assertion)?

This has been changed to a regular map.

---

Comments from Reviewable

[petermattis]

modulo unaddressed comment.

Also, did you do any testing that shows that this reduces the CPU overhead of idle ranges? I believe I filed this issue when doing some scalability testing where I artificially created 500k ranges on a cluster. See the kv/splits/nodes=3 roachtest.

---

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

pkg/storage/replica.go, line 592 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

We've historically had subtle errors in logic like this. Perhaps this code should be restructured for testability. For example, the logic could be placed in a shouldCampaignOnWakeLocked function which takes a lease and other information and returns a boolean.

Ping.

---

Comments from Reviewable

[bdarnell]

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/storage/replica.go, line 592 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ping.

Done

---

Comments from Reviewable

[petermattis]

---

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[bdarnell]

bors r=petermattis

I have not done any testing with large numbers of ranges, FYI.

[craig]

Build succeeded

• GitHub CI (Cockroach)