Merge #74315

74315: kvserver: should support nil references via a pointer receiver on (Lease).String / (Lease)SafeFormat r=kvoli a=kvoli Previously `Lease.String()` `Lease.SafeFormat()` supported value receivers. This caused a panic on passing a nil reference `Lease`. These methods now support nil references via a pointer reciever. Resolves #72814 Release note: None Co-authored-by: Austen McClernon <[email protected]>
cockroachdb · Dec 30, 2021 · cc22c45 · cc22c45
2 parents a6a05fd + 33fe9d1
commit cc22c45
Show file tree

Hide file tree

Showing 12 changed files with 55 additions and 19 deletions.
diff --git a/pkg/kv/kvclient/rangecache/range_cache.go b/pkg/kv/kvclient/rangecache/range_cache.go
@@ -1110,7 +1110,7 @@ type CacheEntry struct {
 }
 
 func (e CacheEntry) String() string {
-	return fmt.Sprintf("desc:%s, lease:%s", e.Desc(), e.lease)
+	return fmt.Sprintf("desc:%s, lease:%s", e.Desc(), &e.lease)
 }
 
 // Desc returns the cached descriptor. Note that, besides being possibly stale,

diff --git a/pkg/kv/kvserver/client_replica_test.go b/pkg/kv/kvserver/client_replica_test.go
@@ -1403,7 +1403,7 @@ func TestRangeLocalUncertaintyLimitAfterNewLease(t *testing.T) {
 		}
 		lease, _ := replica2.GetLease()
 		if lease.Replica.NodeID != replica2.NodeID() {
-			return errors.Errorf("expected lease transfer to node2: %s", lease)
+			return errors.Errorf("expected lease transfer to node2: %s", &lease)
 		}
 		return nil
 	})
@@ -1479,7 +1479,7 @@ func TestLeaseMetricsOnSplitAndTransfer(t *testing.T) {
 		for i := 0; i < 2; i++ {
 			r := tc.GetFirstStoreFromServer(t, i).LookupReplica(roachpb.RKey(expirationKey))
 			if l, _ := r.GetLease(); l.Replica.StoreID != tc.Target(1).StoreID {
-				return errors.Errorf("expected lease to transfer to replica 2: got %s", l)
+				return errors.Errorf("expected lease to transfer to replica 2: got %s", &l)
 			}
 		}
 		return nil

diff --git a/pkg/kv/kvserver/client_split_test.go b/pkg/kv/kvserver/client_split_test.go
@@ -1697,13 +1697,13 @@ func TestStoreSplitTimestampCacheDifferentLeaseHolder(t *testing.T) {
 			return nil
 		}
 		log.Infof(ctx, "received lease request (%s, %s)",
-			leaseReq.Span(), leaseReq.Lease)
+			leaseReq.Span(), &leaseReq.Lease)
 		if !reflect.DeepEqual(*forbiddenDesc, leaseReq.Lease.Replica) {
 			return nil
 		}
 		log.Infof(ctx,
 			"refusing lease request (%s, %s) because %+v held lease for LHS of split",
-			leaseReq.Span(), leaseReq.Lease, forbiddenDesc)
+			leaseReq.Span(), &leaseReq.Lease, forbiddenDesc)
 		return roachpb.NewError(&roachpb.NotLeaseHolderError{RangeID: args.Hdr.RangeID})
 	}
 

diff --git a/pkg/kv/kvserver/replica.go b/pkg/kv/kvserver/replica.go
@@ -940,7 +940,7 @@ func (r *Replica) GetRangeInfo(ctx context.Context) roachpb.RangeInfo {
 			// I wish this could be a Fatal, but unfortunately it's possible for the
 			// lease to be incoherent with the descriptor after a leaseholder was
 			// brutally removed through `cockroach debug unsafe-remove-dead-replicas`.
-			log.Errorf(ctx, "leaseholder replica not in descriptor; desc: %s, lease: %s", desc, l)
+			log.Errorf(ctx, "leaseholder replica not in descriptor; desc: %s, lease: %s", desc, &l)
 			// Let's not return an incoherent lease; for example if we end up
 			// returning it to a client through a br.RangeInfos, the client will freak
 			// out.

diff --git a/pkg/kv/kvserver/replica_closedts_test.go b/pkg/kv/kvserver/replica_closedts_test.go
@@ -628,7 +628,7 @@ func TestRejectedLeaseDoesntDictateClosedTimestamp(t *testing.T) {
 		}
 		lease = li.Current()
 		if !lease.OwnedBy(n2.GetFirstStoreID()) {
-			return errors.Errorf("n2 still unaware of its lease: %s", li.Current())
+			return errors.Errorf("n2 still unaware of its lease: %s", &lease)
 		}
 		return nil
 	})

diff --git a/pkg/kv/kvserver/replica_raft.go b/pkg/kv/kvserver/replica_raft.go
@@ -193,7 +193,7 @@ func (r *Replica) evalAndPropose(
 	} else if !st.Lease.OwnedBy(r.store.StoreID()) {
 		// Perform a sanity check that the lease is owned by this replica. This must
 		// have been ascertained by the callers in checkExecutionCanProceed.
-		log.Fatalf(ctx, "cannot propose %s on follower with remotely owned lease %s", ba, st.Lease)
+		log.Fatalf(ctx, "cannot propose %s on follower with remotely owned lease %s", ba, &st.Lease)
 	} else {
 		proposal.command.ProposerLeaseSequence = st.Lease.Sequence
 	}

diff --git a/pkg/kv/kvserver/replica_range_lease.go b/pkg/kv/kvserver/replica_range_lease.go
@@ -1049,7 +1049,7 @@ func (r *Replica) leaseGoodToGoForStatusRLocked(
 			// this is just a logged error instead of a fatal
 			// assertion.
 			log.Errorf(ctx, "lease %s owned by replica %+v that no longer exists",
-				st.Lease, st.Lease.Replica)
+				&st.Lease, st.Lease.Replica)
 		}
 		// Otherwise, if the lease is currently held by another replica, redirect
 		// to the holder.
@@ -1159,7 +1159,7 @@ func (r *Replica) redirectOnOrAcquireLeaseForRequest(
 					if !stillMember {
 						// See corresponding comment in leaseGoodToGoRLocked.
 						log.Errorf(ctx, "lease %s owned by replica %+v that no longer exists",
-							status.Lease, status.Lease.Replica)
+							&status.Lease, status.Lease.Replica)
 					}
 					// Otherwise, if the lease is currently held by another replica, redirect
 					// to the holder.
@@ -1323,7 +1323,7 @@ func (r *Replica) maybeExtendLeaseAsync(ctx context.Context, st kvserverpb.Lease
 		return
 	}
 	if log.ExpensiveLogEnabled(ctx, 2) {
-		log.Infof(ctx, "extending lease %s at %s", st.Lease, st.Now)
+		log.Infof(ctx, "extending lease %s at %s", &st.Lease, st.Now)
 	}
 	// We explicitly ignore the returned handle as we won't block on it.
 	_ = r.requestLeaseLocked(ctx, st)

diff --git a/pkg/kv/kvserver/replica_rangefeed.go b/pkg/kv/kvserver/replica_rangefeed.go
@@ -716,7 +716,7 @@ func (r *Replica) ensureClosedTimestampStarted(ctx context.Context) *roachpb.Err
 		// In particular, r.redirectOnOrAcquireLease() doesn't work because, if the
 		// current lease is invalid and the current replica is not a leader, the
 		// current replica will not take a lease.
-		log.VEventf(ctx, 2, "ensuring lease for rangefeed range. current lease invalid: %s", lease.Lease)
+		log.VEventf(ctx, 2, "ensuring lease for rangefeed range. current lease invalid: %s", &lease.Lease)
 		err := contextutil.RunWithTimeout(ctx, "read forcing lease acquisition", 5*time.Second,
 			func(ctx context.Context) error {
 				var b kv.Batch

diff --git a/pkg/kv/kvserver/store.go b/pkg/kv/kvserver/store.go
@@ -1501,10 +1501,10 @@ func (s *Store) SetDraining(drain bool, reporter func(int, redact.SafeString)) {
 						if transferStatus != transferOK {
 							if err != nil {
 								log.VErrEventf(ctx, 1, "failed to transfer lease %s for range %s when draining: %s",
-									drainingLeaseStatus.Lease, desc, err)
+									&drainingLeaseStatus.Lease, desc, err)
 							} else {
 								log.VErrEventf(ctx, 1, "failed to transfer lease %s for range %s when draining: %s",
-									drainingLeaseStatus.Lease, desc, transferStatus)
+									&drainingLeaseStatus.Lease, desc, transferStatus)
 							}
 						}
 					}

diff --git a/pkg/roachpb/data.go b/pkg/roachpb/data.go
@@ -1865,12 +1865,12 @@ func (s LeaseSequence) SafeValue() {}
 
 var _ fmt.Stringer = &Lease{}
 
-func (l Lease) String() string {
+func (l *Lease) String() string {
 	return redact.StringWithoutMarkers(l)
 }
 
 // SafeFormat implements the redact.SafeFormatter interface.
-func (l Lease) SafeFormat(w redact.SafePrinter, _ rune) {
+func (l *Lease) SafeFormat(w redact.SafePrinter, _ rune) {
 	if l.Empty() {
 		w.SafeString("<empty>")
 		return
@@ -2528,5 +2528,5 @@ func (ReplicaChangeType) SafeValue() {}
 
 func (ri RangeInfo) String() string {
 	return fmt.Sprintf("desc: %s, lease: %s, closed_timestamp_policy: %s",
-		ri.Desc, ri.Lease, ri.ClosedTimestampPolicy)
+		ri.Desc, &ri.Lease, ri.ClosedTimestampPolicy)
 }
diff --git a/pkg/roachpb/errors.go b/pkg/roachpb/errors.go
@@ -508,7 +508,7 @@ func (e *LeaseRejectedError) Error() string {
 }
 
 func (e *LeaseRejectedError) message(_ *Error) string {
-	return fmt.Sprintf("cannot replace lease %s with %s: %s", e.Existing, e.Requested.String(), e.Message)
+	return fmt.Sprintf("cannot replace lease %s with %s: %s", &e.Existing, e.Requested.String(), e.Message)
 }
 
 var _ ErrorDetailInterface = &LeaseRejectedError{}
@@ -622,7 +622,7 @@ func (e *RangeKeyMismatchError) AppendRangeInfo(
 ) {
 	if !l.Empty() {
 		if _, ok := desc.GetReplicaDescriptorByID(l.Replica.ReplicaID); !ok {
-			log.Fatalf(ctx, "lease names missing replica; lease: %s, desc: %s", l, desc)
+			log.Fatalf(ctx, "lease names missing replica; lease: %s, desc: %s", &l, desc)
 		}
 	}
 	e.Ranges = append(e.Ranges, RangeInfo{

diff --git a/pkg/roachpb/string_test.go b/pkg/roachpb/string_test.go
@@ -142,3 +142,39 @@ func TestSpansString(t *testing.T) {
 		require.Equal(t, tc.expected, tc.spans.String())
 	}
 }
+
+func TestLeaseString(t *testing.T) {
+	for _, tc := range []struct {
+		lease    *roachpb.Lease
+		expected string
+	}{
+		{
+			lease:    &roachpb.Lease{},
+			expected: "<empty>",
+		},
+		{
+			lease:    nil,
+			expected: "<nil>",
+		},
+		{
+			lease: &roachpb.Lease{
+				Replica:    roachpb.ReplicaDescriptor{NodeID: 1, StoreID: 1},
+				Sequence:   1,
+				Start:      hlc.ClockTimestamp(hlc.Timestamp{WallTime: 12, Logical: 123}),
+				Expiration: &hlc.Timestamp{WallTime: 1234, Logical: 12345},
+			},
+			expected: "repl=(n1,s1):? seq=1 start=0.000000012,123 exp=0.000001234,12345",
+		},
+		{
+			lease: &roachpb.Lease{
+				Replica:  roachpb.ReplicaDescriptor{NodeID: 1, StoreID: 1},
+				Sequence: 1,
+				Start:    hlc.ClockTimestamp(hlc.Timestamp{WallTime: 12, Logical: 123}),
+				Epoch:    1,
+			},
+			expected: "repl=(n1,s1):? seq=1 start=0.000000012,123 epo=1",
+		},
+	} {
+		require.Equal(t, tc.expected, tc.lease.String())
+	}
+}