Merge pull request cockroachdb#8283 from tschottdorf/double-desc

storage: do not use non-authoritative Desc()

roachpb/errors.go

@@ -298,7 +298,7 @@ func NewRangeKeyMismatchError(start, end Key, desc *RangeDescriptor) *RangeKeyMi
 	if desc != nil && !desc.IsInitialized() {
 		// We must never send uninitialized ranges back to the client (nil
 		// is fine) guard against regressions of #6027.
-		panic("descriptor is not initialized")
+		panic(fmt.Sprintf("descriptor is not initialized: %+v", desc))
 	}
 	return &RangeKeyMismatchError{
 		RequestStartKey: start,

storage/replica.go

@@ -466,7 +466,7 @@ func (r *Replica) newReplicaInner(desc *roachpb.RangeDescriptor, clock *hlc.Cloc
 
 // String returns a string representation of the range.
 func (r *Replica) String() string {
-	desc := r.Desc()
+	desc := r.InformalDesc()
 	return fmt.Sprintf("%s range=%d [%s-%s)", r.store,
 		desc.RangeID, desc.StartKey, desc.EndKey)
 }
@@ -703,8 +703,19 @@ func (r *Replica) isInitializedLocked() bool {
 	return r.mu.state.Desc.IsInitialized()
 }
 
-// Desc returns the range's descriptor.
+// Desc returns the authoritative range descriptor, acquiring a replica lock in
+// the process.
 func (r *Replica) Desc() *roachpb.RangeDescriptor {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	return r.mu.state.Desc
+}
+
+// InformalDesc returns a cached copy of the authoritative desc which is kept
+// in sync with the authoritative desc, though not atomically. This method
+// does not acquire a lock and is to be used whenever a descriptor is needed
+// for informational purposes.
+func (r *Replica) InformalDesc() *roachpb.RangeDescriptor {
 	return r.rangeDesc.Load().(*roachpb.RangeDescriptor)
 }
 
@@ -737,8 +748,10 @@ func (r *Replica) setDescWithoutProcessUpdateLocked(desc *roachpb.RangeDescripto
 			desc.RangeID, r.RangeID)
 	}
 
-	r.mu.state.Desc = desc
+	// NB: If we used rangeDesc for anything but informational purposes, the
+	// order here would be crucial.
 	r.rangeDesc.Store(desc)
+	r.mu.state.Desc = desc
 }
 
 // GetReplicaDescriptor returns the replica for this range from the range

storage/storagebase/state.proto

@@ -37,7 +37,7 @@ message ReplicaState {
   // The pointer may change, but the referenced RangeDescriptor struct itself
   // must be treated as immutable; it is leaked out of the lock.
   //
-  // Changes of the descriptor should normally go through one of the
+  // Changes of the descriptor should always go through one of the
   // (*Replica).setDesc* methods.
   roachpb.RangeDescriptor desc = 3;
   // The latest lease, if any.

storage/store_test.go

@@ -1062,28 +1062,28 @@ func TestStoreReplicasByKey(t *testing.T) {
 	r4 := splitTestRange(store, roachpb.RKey("X"), roachpb.RKey("ZZ"), t)
 
 	if r := store.LookupReplica(roachpb.RKey("0"), nil); r != r0 {
-		t.Errorf("mismatched range %+v != %+v", r.Desc(), r0.Desc())
+		t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r0.InformalDesc())
 	}
 	if r := store.LookupReplica(roachpb.RKey("B"), nil); r != r1 {
-		t.Errorf("mismatched range %+v != %+v", r.Desc(), r1.Desc())
+		t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r1.InformalDesc())
 	}
 	if r := store.LookupReplica(roachpb.RKey("C"), nil); r != r2 {
-		t.Errorf("mismatched range %+v != %+v", r.Desc(), r2.Desc())
+		t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r2.InformalDesc())
 	}
 	if r := store.LookupReplica(roachpb.RKey("M"), nil); r != r2 {
-		t.Errorf("mismatched range %+v != %+v", r.Desc(), r2.Desc())
+		t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r2.InformalDesc())
 	}
 	if r := store.LookupReplica(roachpb.RKey("X"), nil); r != r3 {
-		t.Errorf("mismatched range %+v != %+v", r.Desc(), r3.Desc())
+		t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r3.InformalDesc())
 	}
 	if r := store.LookupReplica(roachpb.RKey("Z"), nil); r != r3 {
-		t.Errorf("mismatched range %+v != %+v", r.Desc(), r3.Desc())
+		t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r3.InformalDesc())
 	}
 	if r := store.LookupReplica(roachpb.RKey("ZZ"), nil); r != r4 {
-		t.Errorf("mismatched range %+v != %+v", r.Desc(), r4.Desc())
+		t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r4.InformalDesc())
 	}
 	if r := store.LookupReplica(roachpb.RKey("\xff\x00"), nil); r != r4 {
-		t.Errorf("mismatched range %+v != %+v", r.Desc(), r4.Desc())
+		t.Errorf("mismatched range %+v != %+v", r.InformalDesc(), r4.InformalDesc())
 	}
 	if store.LookupReplica(roachpb.RKeyMax, nil) != nil {
 		t.Errorf("expected roachpb.KeyMax to not have an associated range")