kv: RangeKeyMismatchError cleanup.

[mneverov]

kv: RangeKeyMismatchError cleanup.

Fixes #72399

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[cockroach-teamcity]

This change is 

[mneverov]

Reviewable status: complete! 0 of 0 LGTMs obtained

a discussion (no related file):
@nvanbenschoten ptal.

As I understand, the Ranges slice cannot be empty. It still feels weird to access the first element without checking the length.
Let me know if it's better to keep Ranges() method (needs to be renamed then) and have length check there. It would look smth like:

func (e *RangeKeyMismatchError) GetRanges(ctx context.Context) []RangeInfo {
	if len(e.Ranges) == 0 {
		log.Fatalf(ctx, "empty ranges")
	}
	return e.Ranges
}

---

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[nvanbenschoten]

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

a discussion (no related file):
Thanks @mneverov!

It still feels weird to access the first element without checking the length.
Let me know if it's better to keep Ranges() method (needs to be renamed then) and have length check there. It would look smth like:

I agree. We should add some kind of validation here that avoids panicking. After reading through the uses of this error, I'd suggest a slight variation of what you wrote above. In the cases where we need at least one element in the slice, we're always looking for the first element, as this first element has a special meaning. So how about something like:

func (e *RangeKeyMismatchError) MismatchedRange() (RangeInfo, error) {
    if len(e.Ranges) == 0 {
        return RangeInfo{}, errors.AssertionFailedf("RangeKeyMismatchError %+v with empty RangeInfo slice", e)
    }
    return e.Ranges[0], nil
}

---

---

pkg/kv/txn_test.go, line 262 at r2 (raw file):

		{&roachpb.TransactionRetryError{}, true},
		{&roachpb.WriteTooOldError{}, true},
		{&roachpb.RangeNotFoundError{}, false},

Were either of these changes necessary?

---

pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go, line 1426 at r2 (raw file):

		{err: &roachpb.RangeNotFoundError{}, asyncAbort: false},
		{err: &roachpb.RangeKeyMismatchError{
			Ranges: []roachpb.RangeInfo{{}}}, asyncAbort: false,

Was this change necessary?

---

pkg/roachpb/errors.proto, line 95 at r2 (raw file):

  // deprecated_mismatched_range is an older version of mismatched_range. Can go
  // away in 21.1, when we don't worry about 20.1 kvservers not populating rangesInternal.
  optional roachpb.RangeDescriptor deprecated_mismatched_range = 3 [(gogoproto.nullable) = false];

Let's add reserved 3, 4; at the bottom of this proto to ensure that these field numbers are never re-used.

---

pkg/roachpb/errors.proto, line 96 at r2 (raw file):

  // client (always the first in the array) and then all other ranges
  // overlapping the requested key range, or in between the addressed range and
  // the requested key range.

Since we rely on the size of this slice, let's add:

//
// The slice should always have a length of at least 1. However, users should
// call MismatchedRange instead of relying on this in cases where they require
// at least one entry.

[mneverov]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mneverov and @nvanbenschoten)

---

pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go, line 1426 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Was this change necessary?

changes are relevant to the current implementation, without this we get index out of bound error, the same in txn_test.go

---

pkg/roachpb/errors.proto, line 95 at r2 (raw file):

//
// The slice should always have a length of at least 1. However, users should
// call MismatchedRange instead of relying on this in cases where they require
// at least one entry.
👍

---

pkg/roachpb/errors.proto, line 96 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Since we rely on the size of this slice, let's add:

//
// The slice should always have a length of at least 1. However, users should
// call MismatchedRange instead of relying on this in cases where they require
// at least one entry.

Does it make sense then to keep ranges private and expose it via MismatchedRange only? I don't trust comments :)

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mneverov and @nvanbenschoten)

---

pkg/roachpb/errors.proto, line 96 at r2 (raw file):

Previously, mneverov (Max Neverov) wrote…

Does it make sense then to keep ranges private and expose it via MismatchedRange only? I don't trust comments :)

There are cases where we want to access the entire slice and don't make any assumptions about its length, so I think it makes sense to keep the field exported. And I don't expect people will make assumptions about the length and index into the slice blindly without reading the comment.

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[nvanbenschoten]

this looks great. Thanks @mneverov. I'll merge once that last comment is addressed.

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

---

pkg/roachpb/errors.go, line 604 at r3 (raw file):

// MismatchedRange returns the range info for the range that the request was
// erroneously routed to, or an error if the Range slice is empty.

s/Range/Ranges/

[nvanbenschoten]

bors r+

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mneverov)

[craig]

Build succeeded:

• GitHub CI (Cockroach)