kv: Return descriptor hint in RangeKeyMismatchError

[nvanbenschoten]

Resolves #5880.

This change makes Replica perform a best-effort lookup for the correct
range in the case of RangeKeyMismatchError and provide this as a hint
to DistSender for where to look next.

The change doesn't yet include a "replica creation timestamp" as I wasn't
convinced that was needed (but also wasn't convinced it wouldn't be a
nice indicator to have). I'm hoping to get some input on this.

---

This change is

[tbg]

@bdarnell is best qualified to comment on my suggestion about the replica creation timestamp. The reason I brought it up was because I was worried that an old range may not be GCed for a while and, repeatedly being returned with this new code here, could confuse DistSender (which at the moment takes hints pretty seriously). Though it's not clear to me whether it could repeatedly be returned (DistSender would do an authoritative lookup next, so the situation should rectify itself at that point).

---

Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

kv/dist_sender.go, line 667 [r1] (raw file):
s/Front/First/? The term "front" has a direction to it which in my mind suggests it's the c in Scan[a,c) (because a normal scan moves "to the right").

---

kv/dist_sender.go, line 670 [r1] (raw file):
should double-check that [b,c).ContainsKeyRange(b, a) doesn't do unexpected things (i.e. that ContainsKeyRange($1, $2) == false for all $1 >= $2.

---

kv/dist_sender.go, line 731 [r1] (raw file):
Add comment about likely culprit being a rebalance.

---

kv/dist_sender.go, line 750 [r1] (raw file):
Add a small comment that this would likely happen on a Split.

---

kv/dist_sender_test.go, line 660 [r1] (raw file):
Can this new (and somewhat copy-pasta) test replace the one just above? After all, it's testing the same and more. That would make the changes easier to spot as well.

---

kv/range_cache.go, line 232 [r1] (raw file):
nit:

When called without arguments, EvictAndReplace will behave the same as Evict`.

is a bit more correct (there's a newDescs slice, but only kind of).

---

storage/replica.go, line 791 [r1] (raw file):
s/rkme/mismatchErr/ (or anything with an Err suffix)

This is untested as far as I can tell?

---

Comments from Reviewable

[bdarnell]

Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

storage/replica.go, line 797 [r1] (raw file):
Per my comment on #5880, we may want to set SuggestedRange only when a leader is known. (i.e. repl.getLease() is non-nil and lease.Covers(clock.Now()))

---

Comments from Reviewable

[nvanbenschoten]

Review status: 7 of 11 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

kv/dist_sender.go, line 667 [r1] (raw file):
What I was going for here was that during this outer loop we're trying to chop away at the front (in the context of the request direction) of the current key range. First seems to imply left to me, which confuses a reverse search. That said, I completely get where you're coming from. Does "Beginning" work?

---

kv/dist_sender.go, line 670 [r1] (raw file):
As long as the start and end keys aren't inverted, it seems to make sense.

---

kv/dist_sender.go, line 731 [r1] (raw file):
Done.

---

kv/dist_sender.go, line 750 [r1] (raw file):
Done.

---

kv/dist_sender_test.go, line 660 [r1] (raw file):
I dont think they're quite testing the same thing. The one above is testing that after a RangeKeyMismatchError without a suggestion DistSender will perform another LookupRequest. This one is testing that it wont if the RangeKeyMismatchError includes a hint. Seems worthwhile to keep both tests.

---

kv/range_cache.go, line 232 [r1] (raw file):
Done.

---

storage/replica.go, line 791 [r1] (raw file):
Done. And added a test for both the MismatchedRange and SuggestedRange in the RangeKeyMismatchError.

---

storage/replica.go, line 797 [r1] (raw file):
Perfect, that seems like it should work well.

---

Comments from Reviewable

[bdarnell]

LGTM

---

Reviewed 5 of 7 files at r2.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

kv/range_cache.go, line 232 [r2] (raw file):
Get rid of the stray backtick.

---

Comments from Reviewable

[nvanbenschoten]

Review status: 8 of 9 files reviewed at latest revision, 9 unresolved discussions.

---

kv/range_cache.go, line 232 [r2] (raw file):
Done.

---

Comments from Reviewable