Merge #146129 #146219

146129: kvserver: revert add NoReplicasMoved to AdminScatterResponse r=kvoli a=wenyihu6

**Revert "backup: add warning when no replica moved from AdminScatter"**

This commit reverts 519f22c.

We found that the number of replicas moved is not a reliable criterion for DR to
rely on for retries. We're exploring alternative responses. As a simple example,
in a 3-node single-store cluster with RF=3, it is expected for AdminScatter to
move zero replicas. DR should not retry just because numnber of replicas moved
is zero.

Epic: none
Release note: none

---

**Revert "kvpb: add NoReplicasMoved to AdminScatterResponse"**

This commit reverts 8f00544.

We found that the number of replicas moved is not a reliable criterion for DR to
rely on for retries. We're exploring alternative responses. As a simple example,
in a 3-node single-store cluster with RF=3, it is expected for AdminScatter to
move zero replicas. DR should not retry just because numnber of replicas moved
is zero.

Epic: none
Release note: none




146219: roachtest: deflake declarative_schema_changer/job-compatibility-mixed-version r=fqazi a=fqazi

Previously, the job compatibility test could run into flakes because it reduced the GC TTL to one second. This could cause some system database queries to fail during upgrade preconditions. To address this, this patch leaves a 1-minute GC TTL on the system database, which prevents the low TTL from interfering with internal operations for upgrades.

Fixes: #144920

Release note: None

Co-authored-by: wenyihu6 <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>

Author: 3 people

pkg/backup/generative_split_and_scatter_processor.go

@@ -212,26 +212,11 @@ func (s dbSplitAndScatterer) scatter(
 			// throughput.
 			log.Errorf(ctx, "failed to scatter span [%s,%s): %+v",
 				newScatterKey, newScatterKey.Next(), pErr.GoError())
-		} else {
-			// Log at INFO level when scatter request is rejected because the range is
-			// non-empty (range size exceeds req.MaxSize). This is expected during
-			// RESTORE resume.
-			log.Infof(ctx, "failed to scatter span [%s,%s): %+v",
-				newScatterKey, newScatterKey.Next(), pErr.GoError())
 		}
 		return 0, nil
 	}
 
-	scatteredResp, ok := res.(*kvpb.AdminScatterResponse)
-	if !ok {
-		log.Fatalf(ctx, "expected AdminScatterResponse, got %T", res)
-	}
-	if scatteredResp.NoReplicasMoved {
-		// TODO(#144856): DR should consider retrying the scatter if no replicas
-		// were moved.
-		log.Warningf(ctx, "no replicas moved during scatter")
-	}
-	return s.findDestination(scatteredResp), nil
+	return s.findDestination(res.(*kvpb.AdminScatterResponse)), nil
 }
 
 // findDestination returns the node ID of the node of the destination of the

pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go

@@ -57,6 +57,11 @@ func setShortJobIntervalsStep(
 func setShortGCTTLInSystemZoneConfig(
 	ctx context.Context, l *logger.Logger, r *rand.Rand, h *mixedversion.Helper,
 ) error {
+	// Ensure the system database has a longer TTL interval, which is needed to avoid
+	// flakes on system database queries for upgrades.
+	if err := h.Exec(r, "ALTER DATABASE system CONFIGURE ZONE USING gc.ttlseconds=60;"); err != nil {
+		return err
+	}
 	return h.Exec(r, "ALTER RANGE default CONFIGURE ZONE USING gc.ttlseconds = 1;")
 }
 

pkg/kv/kvpb/api.go

@@ -626,7 +626,6 @@ func (r *AdminScatterResponse) combine(_ context.Context, c combinable, _ *Batch
 		r.RangeInfos = append(r.RangeInfos, otherR.RangeInfos...)
 		r.MVCCStats.Add(otherR.MVCCStats)
 		r.ReplicasScatteredBytes += otherR.ReplicasScatteredBytes
-		r.NoReplicasMoved = r.NoReplicasMoved && otherR.NoReplicasMoved
 	}
 	return nil
 }

pkg/kv/kvpb/api.proto

@@ -1974,13 +1974,6 @@ message AdminScatterResponse {
   // scattered
   int64 replicas_scattered_bytes = 5;
 
-  // NoReplicasMoved indicates whether no replicas were moved during the scatter
-  // operation. NB: this needs to be a separate field, since we can't reliably
-  // infer "no replica moved" from ReplicasScatteredBytes == 0. Empty ranges
-  // have stats.Total() == 0 and ReplicasScatteredBytes will be 0 regardless of
-  // whether any replicas were moved.
-  bool no_replicas_moved = 6;
-
   // NB: if you add a field here, don't forget to update combine().
 }
 

pkg/kv/kvserver/replica_command.go

@@ -4266,11 +4266,6 @@ func (r *Replica) adminScatter(
 		// that were moved so the value may not be entirely accurate, but it is
 		// adequate.
 		ReplicasScatteredBytes: stats.Total() * int64(numReplicasMoved),
-		// NB: this needs to be a separate field, since we can't reliably infer
-		// "no replica moved" from ReplicasScatteredBytes == 0. Empty ranges have
-		// stats.Total() == 0 and ReplicasScatteredBytes will be 0 regardless of
-		// whether any replicas were moved.
-		NoReplicasMoved: numReplicasMoved == 0,
 	}, nil
 }