diff --git a/roachpb/errors.go b/roachpb/errors.go index e6e147c59723..4cba67818bcf 100644 --- a/roachpb/errors.go +++ b/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, diff --git a/storage/replica.go b/storage/replica.go index eef7218e11ef..2da122a6ce75 100644 --- a/storage/replica.go +++ b/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 diff --git a/storage/storagebase/state.pb.go b/storage/storagebase/state.pb.go index e468a623aa76..922b88292cba 100644 --- a/storage/storagebase/state.pb.go +++ b/storage/storagebase/state.pb.go @@ -49,7 +49,7 @@ type ReplicaState struct { // 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. Desc *cockroach_roachpb.RangeDescriptor `protobuf:"bytes,3,opt,name=desc" json:"desc,omitempty"` // The latest lease, if any. diff --git a/storage/storagebase/state.proto b/storage/storagebase/state.proto index c10de1baff72..8f4b2b24527b 100644 --- a/storage/storagebase/state.proto +++ b/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. diff --git a/storage/store_test.go b/storage/store_test.go index fabcaa51c4a5..c9cb5e5e8a76 100644 --- a/storage/store_test.go +++ b/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")