storage: ensure nonoverlapping ranges

[rjnn]

processRangeDescriptorUpdateLocked checks to see if there is an
existing range before adding a Replica. We should widen the check to
look for any overlapping ranges. This addresses one of the cases in #7830.

---

This change is

[petermattis]

This needs an associated test. We're already exercising the various conditions for Store.hasOverlappingRange in TestHasOverlappingRange. I think all that is required is a sister test: TestProcessRangeDescriptorUpdate.

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[tamird]

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

---

Comments from Reviewable

[rjnn]

Test added, PTAL. I've verified locally that the test fails with the old version, but let me know if there's some way I should add a failing test pre-change to verify that the test does what we think it does on Circle.

---

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

---

Comments from Reviewable

[bdarnell]

---

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

---

Comments from Reviewable

[tbg]

but let me know if there's some way I should add a failing test pre-change to verify that the test does what we think it does on Circle.

I think that's ok as is.

---

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

---

storage/store.go, line 1815 [r3] (raw file):

  // Add the range and its current stats into metrics.
  s.metrics.replicaCount.Inc(1)

This should be below the check you just edited?

---

storage/store_test.go, line 590 [r3] (raw file):

}

func TestProcessRangeDescriptorUpdate(t *testing.T) {

This test digs into a lot of internals, but I don't really have a good idea of how to change it.

---

storage/store_test.go, line 595 [r3] (raw file):

  defer stopper.Stop()

  // Clobber the existing range so we can test overlaps that aren't KEYMIN or KEYMAX

s/KEYM(IN|AX)/KeyMlower(\1)/

---

storage/store_test.go, line 626 [r3] (raw file):

      abortCache: NewAbortCache(desc.RangeID),
      raftSender: store.ctx.Transport.MakeSender(func(err error, toReplica roachpb.ReplicaDescriptor) {
          log.Warningf("range %d: outgoing raft transport stream to %s closed by the remote: %v", desc.RangeID, toReplica, err)

?

---

storage/store_test.go, line 630 [r3] (raw file):

  }

  // pretend the range is initialized

Capitalization, punctuation.

---

storage/store_test.go, line 631 [r3] (raw file):

  // pretend the range is initialized
  r.mu.state.Desc = desc

Always grab the prescribed locks, even if you think you don't need to.

---

storage/store_test.go, line 632 [r3] (raw file):

  // pretend the range is initialized
  r.mu.state.Desc = desc
  store.mu.uninitReplicas[roachpb.RangeID(3)] = r

avoid this magic number and define a variable above.

---

Comments from Reviewable

[rjnn]

Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

storage/store.go, line 1815 [r3] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

This should be below the check you just edited?

Seems like it should. Moved.

---

storage/store_test.go, line 590 [r3] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

This test digs into a lot of internals, but I don't really have a good idea of how to change it.

@petermattis, do you have any suggestions?

---

storage/store_test.go, line 595 [r3] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

s/KEYM(IN|AX)/KeyMlower(\1)/

Done.

---

storage/store_test.go, line 626 [r3] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

?

Removed.

---

storage/store_test.go, line 630 [r3] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Capitalization, punctuation.

Done.

---

storage/store_test.go, line 631 [r3] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Always grab the prescribed locks, even if you think you don't need to.

Done.

---

storage/store_test.go, line 632 [r3] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

avoid this magic number and define a variable above.

Done.

---

Comments from Reviewable

[tbg]

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

storage/store_test.go, line 633 [r4] (raw file):

  r.mu.state.Desc = desc
  r.mu.Unlock()
  store.mu.uninitReplicas[newRangeID] = r

Lock?

---

Comments from Reviewable

[rjnn]

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

storage/store_test.go, line 633 [r4] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Lock?

Done.

---

Comments from Reviewable

[tbg]

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[petermattis]

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

---

storage/store_test.go, line 637 [r5] (raw file):

  store.mu.Unlock()

  if err := store.processRangeDescriptorUpdate(r); err != (rangeAlreadyExists{r}) {

You should also test a replica for which Replica.IsInitialized() returns false and one which isn't present in uninitReplicas. This can be the same replica, just moving through various states (uninitialized -> not part of uninitReplicas -> overlaps).

---

storage/store_test.go, line 640 [r5] (raw file):

      t.Errorf("Expected processRangeDescriptorUpdate with overlapping keys to fail, got %v", err)
  }

Spurious blank line.

---

Comments from Reviewable

[rjnn]

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

storage/store_test.go, line 637 [r5] (raw file):

Previously, petermattis (Peter Mattis) wrote…

You should also test a replica for which Replica.IsInitialized() returns false and one which isn't present in uninitReplicas. This can be the same replica, just moving through various states (uninitialized -> not part of uninitReplicas -> overlaps).

Done. PTAL. My gut feeling is that it's a little sketchy in how dependent it is on the internal implementations that it's testing, but I don't have the experience to say if this is kosher or not.

---

storage/store_test.go, line 640 [r5] (raw file):

Previously, petermattis (Peter Mattis) wrote…

Spurious blank line.

Done.

---

Comments from Reviewable

[petermattis]

---

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

---

storage/store_test.go, line 637 [r5] (raw file):

Previously, arjunravinarayan (Arjun Ravi Narayan) wrote…

Done. PTAL. My gut feeling is that it's a little sketchy in how dependent it is on the internal implementations that it's testing, but I don't have the experience to say if this is kosher or not.

The test is dependent about the implementation, but it needs to be in order to test the various error conditions. That seems fine to me.

---

storage/store_test.go, line 631 [r6] (raw file):

  expectedResult := errors.Errorf("attempted to process uninitialized range %s", r)
  if err := store.processRangeDescriptorUpdate(r); err.Error() != expectedResult.Error() {

FYI, testutils.IsError is useful for these kind of checks.

---

storage/store_test.go, line 632 [r6] (raw file):

  expectedResult := errors.Errorf("attempted to process uninitialized range %s", r)
  if err := store.processRangeDescriptorUpdate(r); err.Error() != expectedResult.Error() {
      t.Errorf("Expected processRangeDescriptorUpdate with uninitialized replica to fail, got %v", err)

s/Expected/expected/g (error messages are generally not capitalized)

---

Comments from Reviewable

[rjnn]

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

storage/store_test.go, line 632 [r6] (raw file):

Previously, petermattis (Peter Mattis) wrote…

s/Expected/expected/g (error messages are generally not capitalized)

Done.

---

Comments from Reviewable