kvserver: Use response data in the load-based splitter

[KaiSun314]

Fixes #87279

We investigated why running YCSB Workload E results in a single hot
range and we observed that range queries of the form
SELECT * FROM table WHERE pkey >= A LIMIT B will result in all request
spans having the same end key - similar to [A, range_end) - rather than
end keys that take into account the specified LIMIT. Since the majority
of request spans have the same end key, the load splitter algorithm
cannot find a split key without too many contained and balance between
left and right requests. A proposed solution is to use the response span
rather than the request span, since the response span is more accurate
in reflecting the keys that this request truly iterated over. We utilize
the request span as well as the response's resume span to derive the key
span that this request truly iterated over. Using response data (resume
span) rather than just the request span in the load-based splitter
(experimentally) allows the load-based splitter to find a split key
under range query workloads (YCSB Workload E, KV workload with spans).

Ops/sec for YCSB-E workload with / without this change and various number of nodes (3 / 5) and CPUs (8 / 32): https://docs.google.com/spreadsheets/d/1OcvRUkXORiGpr-f7cMAiuv9DW7qQZgconfqE4UbfQ2c/edit?usp=sharing

Release note (ops change): We use response data rather than just the
request span in the load-based splitter to pass more accurate data
about the keys iterated over to the load splitter to find a suitable
split key, enabling the load splitter to find a split key under heavy
range query workloads.

[cockroach-teamcity]

This change is 

[kvoli]

Nice patch. Can you post the results of running ycsb-e for the unsplittable metrics and workload ops/s when you get a chance.

Going to tag @aayushshah15 to also take a look.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @KaiSun314)

---

pkg/kv/kvserver/replica_send.go line 405 at r1 (raw file):

					// Request:    [key...............endKey]
					// ResumeSpan:          [key......endKey]
					// True span:  [key......key]

nit: for clarity, add in [key,endKey) rather than []. To avoid any confusion about theendKey being inclusive when it is exclusive.

Code quote:

					// Request:    [key...............endKey]
					// ResumeSpan:          [key......endKey]
					// True span:  [key......key]

[KaiSun314]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15 and @kvoli)

---

pkg/kv/kvserver/replica_send.go line 405 at r1 (raw file):

Previously, kvoli (Austen) wrote…

nit: for clarity, add in [key,endKey) rather than []. To avoid any confusion about theendKey being inclusive when it is exclusive.

Done. Changed to [key, endKey). Also changed for reverse scan comment below, just want to confirm that reverse scans are also exclusive [key, endKey)?

[kvoli]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15)

---

pkg/kv/kvserver/replica_send.go line 405 at r1 (raw file):

Previously, KaiSun314 (Kai Sun) wrote…

Done. Changed to [key, endKey). Also changed for reverse scan comment below, just want to confirm that reverse scans are also exclusive [key, endKey)?

That's right, take a look at this comment. It is specific to spans:

https://github.com/cockroachdb/cockroach/blob/20e55fcca7651472a6b112900c413e67b270979f/pkg/roachpb/data.pb.go#L285-L288

[aayushshah15]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @KaiSun314)

---

pkg/kv/kvserver/replica_send.go line 381 at r2 (raw file):

var _ batchExecutionFn = (*Replica).executeReadOnlyBatch

func getTrueSpans(

This function re-implements the logic represented by Replica.collectSpansRead(). Let's reuse that method here. Also note the special handling for SkipLocked requests that is dealt with in collectSpansRead. Making this change should simplify this patch significantly.

[KaiSun314]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15 and @kvoli)

---

pkg/kv/kvserver/replica_send.go line 381 at r2 (raw file):

Previously, aayushshah15 (Aayush Shah) wrote…

This function re-implements the logic represented by Replica.collectSpansRead(). Let's reuse that method here. Also note the special handling for SkipLocked requests that is dealt with in collectSpansRead. Making this change should simplify this patch significantly.

Done.

For SkipLocked requests, my understanding is that the spans returned will contain one span for each key in the response. I think using Replica.collectSpansRead() in this case should still be fine, since we take the union of all the spans returned?

[KaiSun314]

TFTRs!

Bazel Extended CI test failures appear to be unrelated.

bors r+

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[KaiSun314]

bors r+

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[KaiSun314]

bors r+

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[kvoli]

Try rebasing on master - it looks like a persistent issue with that test that is unrelated.

[knz]

alas master is where the problem is. That's why the bors run failed and not the original CI on this PR.

[KaiSun314]

bors r+

[KaiSun314]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)