Merge pull request #14312 from a-robinson/logging

storage: Some logging/comment improvements

pkg/storage/allocator.go

@@ -485,21 +485,26 @@ func (a *Allocator) ShouldTransferLease(
 	sl, _, _ := a.storePool.getStoreList(rangeID)
 	sl = sl.filter(constraints)
 	if log.V(3) {
-		log.Infof(ctx, "ShouldTransferLease source (lease-holder=%d):\n%s", leaseStoreID, sl)
+		log.Infof(ctx, "ShouldTransferLease (lease-holder=%d):\n%s", leaseStoreID, sl)
 	}
 
 	transferDec, _ := a.shouldTransferLeaseUsingStats(ctx, sl, source, existing, stats)
+	var result bool
 	switch transferDec {
 	case shouldNotTransfer:
-		return false
+		result = false
 	case shouldTransfer:
-		return true
+		result = true
 	case decideWithoutStats:
+		result = a.shouldTransferLeaseWithoutStats(ctx, sl, source, existing)
 	default:
 		log.Fatalf(ctx, "unexpected transfer decision %d", transferDec)
 	}
 
-	return a.shouldTransferLeaseWithoutStats(ctx, sl, source, existing)
+	if log.V(3) {
+		log.Infof(ctx, "ShouldTransferLease decision (lease-holder=%d): %t", leaseStoreID, result)
+	}
+	return result
 }
 
 func (a Allocator) shouldTransferLeaseUsingStats(

pkg/storage/replica.go

@@ -763,8 +763,8 @@ func (r *Replica) setReplicaIDLocked(replicaID roachpb.ReplicaID) error {
 	}
 	if replicaID == 0 {
 		// If the incoming message does not have a new replica ID it is a
-		// preemptive snapshot. We'll set a tombstone for the old replica ID if the
-		// snapshot is accepted.
+		// preemptive snapshot. We'll update minReplicaID if the snapshot is
+		// accepted.
 		return nil
 	}
 	if replicaID < r.mu.minReplicaID {
@@ -785,6 +785,10 @@ func (r *Replica) setReplicaIDLocked(replicaID roachpb.ReplicaID) error {
 	// If there was a previous replica, repropose its pending commands under
 	// this new incarnation.
 	if previousReplicaID != 0 {
+		if log.V(1) {
+			log.Infof(r.AnnotateCtx(context.TODO()), "changed replica ID from %d to %d",
+				previousReplicaID, replicaID)
+		}
 		// repropose all pending commands under new replicaID.
 		r.refreshProposalsLocked(0, reasonReplicaIDChanged)
 	}

pkg/storage/replica_gc_queue.go

@@ -198,7 +198,7 @@ func (rgcq *replicaGCQueue) process(
 	}
 
 	replyDesc := reply.Ranges[0]
-	if _, currentMember := replyDesc.GetReplicaDescriptor(repl.store.StoreID()); !currentMember {
+	if currentDesc, currentMember := replyDesc.GetReplicaDescriptor(repl.store.StoreID()); !currentMember {
 		// We are no longer a member of this range; clean up our local data.
 		rgcq.metrics.RemoveReplicaCount.Inc(1)
 		if log.V(1) {
@@ -231,7 +231,7 @@ func (rgcq *replicaGCQueue) process(
 		// Replica (see #8111) when inactive ones can be starved by
 		// event-driven additions.
 		if log.V(1) {
-			log.Infof(ctx, "not gc'able")
+			log.Infof(ctx, "not gc'able, replica is still in range descriptor: %v", currentDesc)
 		}
 		if err := repl.setLastReplicaGCTimestamp(ctx, repl.store.Clock().Now()); err != nil {
 			return err

pkg/storage/store.go

@@ -2764,6 +2764,9 @@ func (s *Store) HandleSnapshot(header *SnapshotRequest_Header, stream SnapshotRe
 	if err := stream.Send(&SnapshotResponse{Status: SnapshotResponse_ACCEPTED}); err != nil {
 		return err
 	}
+	if log.V(2) {
+		log.Infof(ctx, "accepted snapshot reservation for r%d", header.State.Desc.RangeID)
+	}
 
 	var batches [][]byte
 	var logEntries [][]byte
@@ -2988,14 +2991,14 @@ func (s *Store) processRaftRequest(
 	}
 
 	// Snapshots addressed to replica ID 0 are permitted; this is the
-	// mechanism by which preemptive snapshots work.
+	// mechanism by which preemptive snapshots work. No other requests to
+	// replica ID 0 are allowed.
 	//
-	// No other requests to replica ID 0 are allowed.
+	// Note that just because the ToReplica's ID is 0 it does not necessarily
+	// mean that the replica's current ID is 0. We allow for preemptive snaphots
+	// to be applied to initialized replicas as of #8613.
 	if req.ToReplica.ReplicaID == 0 {
 		if req.Message.Type != raftpb.MsgSnap {
-			// We disallow non-snapshot messages to replica ID 0. Note that
-			// getOrCreateReplica disallows moving the replica ID backward, so the
-			// only way we can get here is if the replica did not previously exist.
 			log.VEventf(ctx, 1, "refusing incoming Raft message %s from %+v to %+v",
 				req.Message.Type, req.FromReplica, req.ToReplica)
 			return roachpb.NewErrorf(