Merge #152194 #152271

152194: kvserver: update splitPreApply comments r=arulajmani a=pav-kv

Epic: CRDB-49111

152271: asim: Revert "asim: fix dd output" r=sumeerbhola,stevendanna a=wenyihu6

This reverts commit 568bea8.

Previously, we regenerated asim data driven output in 568bea8 since 8f1144f
modified our datadriven output. Since 8f1144f was later reverted on master (see
#152023 ), this commit reverts
568bea8 as well.

Fixes: #152221
Release note: none

Co-authored-by: Pavel Kalinnikov <[email protected]>
Co-authored-by: wenyihu6 <[email protected]>

Author: 3 people

pkg/kv/kvserver/asim/tests/testdata/non_rand/example_zone_config.txt

@@ -27,6 +27,6 @@ assertion type=stat stat=replicas ticks=5 exact_bound=0 stores=(13,14,15,16,17,1
 
 eval duration=10m samples=1 seed=42 metrics=(replicas)
 ----
-replicas#1: first: [s1=17, s2=17, s3=17, s4=17, s5=17, s6=16, s7=17, s8=16, s9=16, s10=17, s11=17, s12=16, s13=17, s14=17, s15=17, s16=16, s17=17, s18=17, s19=17, s20=17, s21=16, s22=16, s23=17, s24=17, s25=17, s26=17, s27=17, s28=17, s29=16, s30=17, s31=16, s32=17, s33=16, s34=16, s35=17, s36=16] (stddev=0.47, mean=16.67, sum=600)
-replicas#1: last:  [s1=50, s2=50, s3=50, s4=49, s5=50, s6=50, s7=51, s8=50, s9=51, s10=50, s11=49, s12=50, s13=0, s14=0, s15=0, s16=0, s17=0, s18=0, s19=0, s20=0, s21=0, s22=0, s23=0, s24=0, s25=0, s26=0, s27=0, s28=0, s29=0, s30=0, s31=0, s32=0, s33=0, s34=0, s35=0, s36=0] (stddev=23.57, mean=16.67, sum=600)
-artifacts[default]: fb6d3b238ce25bbb
+replicas#1: first: [s1=17, s2=17, s3=17, s4=17, s5=17, s6=16, s7=16, s8=17, s9=17, s10=17, s11=17, s12=17, s13=16, s14=16, s15=16, s16=16, s17=17, s18=17, s19=17, s20=16, s21=17, s22=16, s23=17, s24=17, s25=17, s26=17, s27=17, s28=16, s29=17, s30=16, s31=17, s32=17, s33=16, s34=16, s35=17, s36=17] (stddev=0.47, mean=16.67, sum=600)
+replicas#1: last:  [s1=51, s2=50, s3=52, s4=51, s5=49, s6=49, s7=50, s8=49, s9=49, s10=50, s11=48, s12=52, s13=0, s14=0, s15=0, s16=0, s17=0, s18=0, s19=0, s20=0, s21=0, s22=0, s23=0, s24=0, s25=0, s26=0, s27=0, s28=0, s29=0, s30=0, s31=0, s32=0, s33=0, s34=0, s35=0, s36=0] (stddev=23.58, mean=16.67, sum=600)
+artifacts[default]: 48fc2e7a0c77951b

pkg/kv/kvserver/asim/tests/testdata/non_rand/mma_high_cpu_25nodes.txt

@@ -25,10 +25,10 @@ gen_load rate=15000 rw_ratio=0.95 min_block=1 max_block=1 request_cpu_per_access
 
 eval duration=25m samples=1 seed=42 cfgs=(mma-only) metrics=(cpu,write_bytes_per_second,replicas,leases)
 ----
-cpu#1: last:  [s1=74658907, s2=68208820, s3=62841720, s4=61883151, s5=61568465, s6=60936387, s7=60869228, s8=61108420, s9=60824385, s10=60846480, s11=60692919, s12=60819115, s13=60534302, s14=60932892, s15=60802021, s16=60645005, s17=60688046, s18=61029928, s19=61026148, s20=61068941, s21=60733024, s22=60686783, s23=60937532, s24=60830854, s25=60950526] (stddev=3007243.57, mean=61844959.96, sum=1546123999)
-leases#1: first: [s1=29, s2=13, s3=4, s4=2, s5=2, s6=2, s7=3, s8=3, s9=2, s10=3, s11=4, s12=1, s13=2, s14=2, s15=4, s16=3, s17=3, s18=3, s19=2, s20=3, s21=3, s22=2, s23=2, s24=1, s25=2] (stddev=5.56, mean=4.00, sum=100)
-leases#1: last:  [s1=4, s2=4, s3=4, s4=3, s5=4, s6=4, s7=5, s8=4, s9=4, s10=4, s11=5, s12=3, s13=3, s14=4, s15=5, s16=4, s17=4, s18=5, s19=4, s20=4, s21=4, s22=4, s23=4, s24=3, s25=4] (stddev=0.57, mean=4.00, sum=100)
-replicas#1: first: [s1=56, s2=44, s3=25, s4=16, s5=11, s6=9, s7=8, s8=8, s9=7, s10=8, s11=7, s12=7, s13=7, s14=7, s15=7, s16=7, s17=7, s18=7, s19=7, s20=8, s21=8, s22=7, s23=8, s24=7, s25=7] (stddev=11.96, mean=12.00, sum=300)
-replicas#1: last:  [s1=55, s2=33, s3=16, s4=12, s5=11, s6=9, s7=9, s8=9, s9=9, s10=9, s11=8, s12=9, s13=8, s14=9, s15=8, s16=8, s17=8, s18=9, s19=9, s20=9, s21=8, s22=8, s23=9, s24=9, s25=9] (stddev=10.07, mean=12.00, sum=300)
-write_bytes_per_second#1: last:  [s1=9724, s2=9367, s3=9131, s4=9033, s5=9048, s6=9046, s7=9017, s8=9057, s9=9060, s10=9071, s11=8996, s12=9034, s13=9015, s14=9053, s15=9061, s16=9067, s17=9074, s18=9059, s19=9031, s20=9043, s21=9056, s22=9026, s23=9065, s24=9036, s25=9066] (stddev=145.82, mean=9089.44, sum=227236)
-artifacts[mma-only]: ec4f96c58ffca54
+cpu#1: last:  [s1=74125827, s2=68034597, s3=63030218, s4=62464586, s5=61575256, s6=60952144, s7=60895605, s8=60583595, s9=60898231, s10=60917161, s11=60720570, s12=60655112, s13=61045604, s14=60959500, s15=61069491, s16=60785250, s17=60803274, s18=60714988, s19=60866275, s20=60670002, s21=60640246, s22=60684341, s23=60958310, s24=61291868, s25=60628308] (stddev=2911609.33, mean=61838814.36, sum=1545970359)
+leases#1: first: [s1=28, s2=14, s3=4, s4=3, s5=3, s6=2, s7=2, s8=2, s9=2, s10=2, s11=3, s12=2, s13=3, s14=3, s15=2, s16=3, s17=3, s18=3, s19=2, s20=2, s21=2, s22=3, s23=3, s24=3, s25=1] (stddev=5.43, mean=4.00, sum=100)
+leases#1: last:  [s1=3, s2=4, s3=4, s4=4, s5=4, s6=4, s7=4, s8=3, s9=4, s10=4, s11=5, s12=3, s13=5, s14=4, s15=4, s16=4, s17=4, s18=4, s19=4, s20=4, s21=4, s22=4, s23=5, s24=5, s25=3] (stddev=0.57, mean=4.00, sum=100)
+replicas#1: first: [s1=56, s2=44, s3=25, s4=16, s5=12, s6=9, s7=8, s8=7, s9=7, s10=7, s11=7, s12=7, s13=7, s14=8, s15=7, s16=8, s17=7, s18=8, s19=7, s20=7, s21=7, s22=7, s23=7, s24=8, s25=7] (stddev=11.97, mean=12.00, sum=300)
+replicas#1: last:  [s1=53, s2=33, s3=16, s4=14, s5=11, s6=9, s7=9, s8=8, s9=9, s10=9, s11=8, s12=8, s13=9, s14=9, s15=9, s16=9, s17=8, s18=8, s19=9, s20=9, s21=8, s22=8, s23=9, s24=10, s25=8] (stddev=9.74, mean=12.00, sum=300)
+write_bytes_per_second#1: last:  [s1=9710, s2=9402, s3=9119, s4=9121, s5=9065, s6=9029, s7=9020, s8=9027, s9=9065, s10=9049, s11=9012, s12=8981, s13=9057, s14=9058, s15=9086, s16=9069, s17=9029, s18=9030, s19=9035, s20=9020, s21=9031, s22=9035, s23=9054, s24=9084, s25=9050] (stddev=147.55, mean=9089.52, sum=227238)
+artifacts[mma-only]: c6076cf40636a701

pkg/kv/kvserver/store_split.go

@@ -46,12 +46,14 @@ func splitPreApply(
 			r.StoreID(), split)
 	}
 
-	// Check on the RHS, we need to ensure that it exists and has a minReplicaID
-	// less than or equal to the replica we're about to initialize.
+	// Obtain the RHS replica. In the common case, it exists and its ReplicaID
+	// matches the one in the split trigger. It is the uninitialized replica that
+	// has just been created or obtained in Replica.acquireSplitLock, and its
+	// raftMu is locked.
 	//
-	// The right hand side of the split was already created (and its raftMu
-	// acquired) in Replica.acquireSplitLock. It must be present here if it hasn't
-	// been removed in the meantime (handled below).
+	// In the less common case, the ReplicaID is already removed from this Store,
+	// and rightRepl is either nil or an uninitialized replica with a higher
+	// ReplicaID. Its raftMu is not locked.
 	rightRepl := r.store.GetReplicaIfExists(split.RightDesc.RangeID)
 	// Check to see if we know that the RHS has already been removed from this
 	// store at the replica ID implied by the split.
@@ -76,6 +78,8 @@ func splitPreApply(
 		// it's always present).
 		var hs raftpb.HardState
 		if rightRepl != nil {
+			// TODO(pav-kv): rightRepl could have been destroyed by the time we get to
+			// lock it here. The HardState read-then-write appears risky in this case.
 			rightRepl.raftMu.Lock()
 			defer rightRepl.raftMu.Unlock()
 			// Assert that the rightRepl is not initialized. We're about to clear out
@@ -90,6 +94,10 @@ func splitPreApply(
 				log.Fatalf(ctx, "failed to load hard state for removed rhs: %v", err)
 			}
 		}
+		// TODO(#152199): the rightRepl == nil condition is flaky. There can be a
+		// racing replica creation for a higher ReplicaID, and it can subsequently
+		// update its HardState. Here, we can accidentally clear the HardState of
+		// that new replica.
 		if err := kvstorage.ClearRangeData(ctx, split.RightDesc.RangeID, readWriter, readWriter, kvstorage.ClearRangeDataOptions{
 			// We know there isn't anything in these two replicated spans below in the
 			// right-hand side (before the current batch), so setting these options
@@ -99,19 +107,17 @@ func splitPreApply(
 			ClearReplicatedByRangeID: true,
 			// See the HardState write-back dance above and below.
 			//
-			// TODO(tbg): we don't actually want to touch the raft state of the right
-			// hand side replica since it's absent or a more recent replica than the
-			// split. Now that we have a boolean targeting the unreplicated
-			// RangeID-based keyspace, we can set this to false and remove the
-			// HardState+ReplicaID write-back. (The WriteBatch does not contain
-			// any writes to the unreplicated RangeID keyspace for the RHS, see
-			// splitTriggerHelper[^1]).
+			// TODO(tbg): we don't actually want to touch the raft state of the RHS
+			// replica since it's absent or a more recent one than in the split. Now
+			// that we have a bool targeting unreplicated RangeID-local keys, we can
+			// set it to false and remove the HardState+ReplicaID write-back. However,
+			// there can be historical split proposals with the RaftTruncatedState key
+			// set in splitTriggerHelper[^1]. We must first make sure that such
+			// proposals no longer exist, e.g. with a below-raft migration.
 			//
 			// [^1]: https://github.com/cockroachdb/cockroach/blob/f263a765d750e41f2701da0a923a6e92d09159fa/pkg/kv/kvserver/batcheval/cmd_end_transaction.go#L1109-L1149
 			//
-			// See also:
-			//
-			// https://github.com/cockroachdb/cockroach/issues/94933
+			// See also: https://github.com/cockroachdb/cockroach/issues/94933
 			ClearUnreplicatedByRangeID: true,
 		}); err != nil {
 			log.Fatalf(ctx, "failed to clear range data for removed rhs: %v", err)