storage: Don't require splits to complete before running replicate queue

[a-robinson]

Currently, the replicate queue requires that all splits complete before doing its thing (

cockroach/pkg/storage/replicate_queue.go

Line 117 in 5d39b64

acceptsUnsplitRanges: store.TestingKnobs().ReplicateQueueAcceptsUnsplit,

,

cockroach/pkg/storage/replicate_queue.go

Line 152 in 5d39b64

if !repl.store.splitQueue.Disabled() && repl.needsSplitBySize() {

).

This makes sense, because it's cheaper/less risky to send snapshots for smaller ranges than for ones that are bigger than the configured limit. However, this leaves the cluster susceptible to data unavailability/loss if a range fails to ever split for some reason. If we had a perfect track record of splits never getting stuck, this wouldn't be a big deal, but we have had some problems with ranges failing to split (e.g., #24966, #25036, #24896, #23310, #21357).

I suspect we'd be better off allowing replication even if size-based splitting was needed. Do you have opinions, @nvanbenschoten or @tschottdorf?

[tbg]

Snapshots are currently pulled into memory on the receiving side, which I think is one of the original motivations for this restriction. With @nvanbenschoten's upcoming change and possible elimination of other memory-blowup inducing code, we may be able to drop this coupling again, and I absolutely think we should do so once it becomes safe.

[nvanbenschoten]

I don't know the exact reason for introducing this restriction, but I suspect there were two main factors:

• ranges had no upper bound on size, so if they needed a split then that means they could have been arbitrarily large.
• large snapshots are expensive and potentially deadly because they buffer the entire range in memory.

The first factor is similar to the reason why we used to prevent snapshots when the range size was larger than 2x the max_range_size. We removed this restriction after we introduced backpressure that attempted to bound the size of ranges. The second factor is still valid, but will be addressed in #16954. Since we do a better job bounding range size and will soon remove the effective snapshot size limit, I think we can make this change shortly.

Whether we should make the change is another question. I personally would love to see it because it reduces the number of components that have dependencies on each other. As we saw in #20589, these dependencies can easily turn into cycles which create deadlocks, so it's almost always best to avoid them.

[a-robinson]

I don't know the exact reason for introducing this restriction, but I suspect there were two main factors:

Those are definitely the factors -- there's a comment in the code I linked to that explicitly lays that out. Sorry for not including the full range of relevant lines in the original post:

cockroach/pkg/storage/replicate_queue.go

Lines 153 to 157 in 5d39b64

	// If the range exceeds the split threshold, let that finish first.
	// Ranges must fit in memory on both sender and receiver nodes while
	// being replicated. This supplements the check provided by
	// acceptsUnsplitRanges, which looks at zone config boundaries rather
	// than data size.

Whether we should make the change is another question.

That question is easy. Once the large snapshot problem is addressed, we should certainly remove the repl.needsSplitBySize() check. As is, splits not working can lead to under-replication/unavailability/data loss because we won't ever up-replicate the range even if nodes are lost.

[nvanbenschoten]

@andreimatei you just addressed this in #38529, right?

[andreimatei]

yup