storage: correctly gc locally-anchored transactions

[tbg]

The previous way of computing the key range for the GCRequest was incorrectly
making the assumption that (roachpb.Key).Next respected ordering on the
corresponding addressing-resolved keys.

This lead to intents on RangeDescriptors not being resolved during range splits
and perhaps rebalances, as observed in #8978.

---

This change is

[petermattis]

---

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[tamird]

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

storage/intent_resolver.go, line 347 [r1] (raw file):

              //
              // #7880 will address this by making GCRequest less special and
              // thus obviating the need to cook up an artificial range here.

s/obviating/obviate/

probably want to have the string "TODO" somewhere in here.

---

storage/replica_command.go, line 791 [r1] (raw file):

  if bytes.Compare(span.Key, keys.LocalRangeMax) < 0 {
      if bytes.Compare(span.EndKey, keys.LocalRangeMax) >= 0 {
          log.Fatalf(context.Background(), "a local intent range may not have a non-local portion: %s", span)

why not TODO?

---

storage/replica_test.go, line 3289 [r1] (raw file):

          defer tc.Stop()

          ctx := log.WithLogTag(context.Background(), "testcase", i)

👍 neat

---

Comments from Reviewable

[tbg]

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

---

storage/intent_resolver.go, line 347 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/obviating/obviate/

probably want to have the string "TODO" somewhere in here.

Won't open a PR for this, but will keep in mind.

---

storage/replica_command.go, line 791 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why not TODO?

That's not a TODO, it simply must not happen. This indicates that some broken key range such as [/RangeDescriptor/"a", "some user key"). A key range is either completely local or completely non-local.

---

Comments from Reviewable

[tamird]

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

---

storage/replica_command.go, line 791 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

That's not a TODO, it simply must not happen. This indicates that some broken key range such as [/RangeDescriptor/"a", "some user key"). A key range is either completely local or completely non-local.

I meant `context.TODO()`. If this happens (and of course, it must not), you might want to pass a context with some....context.

---

Comments from Reviewable