storage: add unsplittable ranges to split queue purgatory

[nvanbenschoten]

Fixes #25191.

This makes the fix for #24215 trivial.

In #14654 we added a mechanism to double the max range size whenever a
split attempt found a range that was unsplittable. This prevented a
tight loop of split attempts. However, it didn't actually do anything to
help us find a split point to reduce the size of the range in the
future. This size doubling worked in practice, but it was a blunt
instrument that had strange effects (see #24215).

This change rips out this range size doubling and replaces it with a
split queue purgatory. This purgatory is used to house replicas that
are unsplittable, preventing them from getting into a tight loop.

Release note: None

[cockroach-teamcity]

This change is 

[bdarnell]

---

Reviewed 10 of 10 files at r1, 1 of 1 files at r2, 7 of 7 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

pkg/storage/replica_command.go, line 740 at r3 (raw file):

		// On seeing a ConditionFailedError or an AmbiguousResultError, retry the
		// command with the updated descriptor.
		switch errors.Cause(lastErr).(type) {

I don't like mixing the error and roachpb.Error domains here. I think I'd rather make unsplittableRangeError a roachpb error so we can use the same introspection mode for it as for the other errors.

---

pkg/storage/split_queue.go, line 40 at r3 (raw file):

	// unsplittable because they do not contain a suitable split key. Purgatory
	// prevents them from repeatedly attempting to split at an unbounded rate.
	splitQueuePurgatoryCheckInterval = 10 * time.Minute

This seems like a long time. I'd make this 1 minute instead.

---

Comments from Reviewable

[nvanbenschoten]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

pkg/storage/replica_command.go, line 740 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I don't like mixing the error and roachpb.Error domains here. I think I'd rather make unsplittableRangeError a roachpb error so we can use the same introspection mode for it as for the other errors.

If we do that then we would need to have a roachpb error implement purgatoryError, which seems wrong to me. With the way this is structured now, splitQueue.process never enters the roachpb.Error domain, which seems like the right approach to me.

---

pkg/storage/split_queue.go, line 40 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This seems like a long time. I'd make this 1 minute instead.

Done.

---

Comments from Reviewable

[bdarnell]

Reviewed 7 of 7 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

pkg/storage/replica_command.go, line 740 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

If we do that then we would need to have a roachpb error implement purgatoryError, which seems wrong to me. With the way this is structured now, splitQueue.process never enters the roachpb.Error domain, which seems like the right approach to me.

Yeah, I guess that would mean exporting all of the purgatory errors to roachpb, which doesn't seem worth it. This is fine, then.

---

Comments from Reviewable

[nvanbenschoten]

TFTR!

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)