Merge cockroachdb#114270

114270: kvserver: increase write load acct test allowance by 1 r=andrewbaptist a=kvoli

`TestWriteLoadStatsAccounting` asserts that the replica load tracker matches the sent write counts. Flakes can occur due to retries, interleaving requests, etc. Bump the error allowance from 4 to 5.

Resolves: cockroachdb#114239
Release note: None

Co-authored-by: Austen McClernon <austen@cockroachlabs.com>

pkg/kv/kvserver/replica_rankings_test.go

@@ -24,7 +24,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/load"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage"
-	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/storageutils"
@@ -188,7 +187,7 @@ func TestAddSSTQPSStat(t *testing.T) {
 		// If queries are correctly recorded, we should see increase in query
 		// count by the expected QPS. However, it is possible to to get a
 		// slightly higher number due to interleaving requests. To avoid a
-		// flakey test, we assert that QPS is at least as high as expected,
+		// flaky test, we assert that QPS is at least as high as expected,
 		// then no greater than 4 requests of expected QPS. If this test is
 		// flaky, increase the delta to account for background activity
 		// interleaving with measurements.
@@ -221,7 +220,7 @@ func TestWriteLoadStatsAccounting(t *testing.T) {
 	args.ServerArgs.Knobs.Store = &StoreTestingKnobs{DisableCanAckBeforeApplication: true}
 	tc := serverutils.StartCluster(t, 1, args)
 
-	const epsilonAllowed = 4
+	const epsilonAllowed = 5
 
 	defer tc.Stopper().Stop(ctx)
 	ts := tc.Server(0)
@@ -259,48 +258,40 @@ func TestWriteLoadStatsAccounting(t *testing.T) {
 	sqlDB.Exec(t, `SET CLUSTER SETTING kv.range_split.by_load.enabled = false`)
 
 	for _, testCase := range testCases {
-		// This test can flake, where an errant request - not sent here
-		// (commonly intent resolution) will artifically inflate the collected
-		// metrics. This results in unexpected read/write statistics and a
-		// flakey test every few hundred runs. Here we assert that the run
-		// should succeed soon, if it fails on the first.
-		testutils.SucceedsSoon(t, func() error {
-			// Reset the request counts to 0 before sending to clear previous requests.
-			repl.loadStats.Reset()
-
-			requestsBefore := repl.loadStats.TestingGetSum(load.Requests)
-			writesBefore := repl.loadStats.TestingGetSum(load.WriteKeys)
-			readsBefore := repl.loadStats.TestingGetSum(load.ReadKeys)
-			readBytesBefore := repl.loadStats.TestingGetSum(load.ReadBytes)
-			writeBytesBefore := repl.loadStats.TestingGetSum(load.WriteBytes)
-
-			for i := 0; i < testCase.writes; i++ {
-				_, pErr := db.Inc(ctx, scratchKey, 1)
-				require.Nil(t, pErr)
-			}
-			require.Equal(t, 0.0, requestsBefore)
-			require.Equal(t, 0.0, writesBefore)
-			require.Equal(t, 0.0, readsBefore)
-			require.Equal(t, 0.0, writeBytesBefore)
-			require.Equal(t, 0.0, readBytesBefore)
-
-			requestsAfter := repl.loadStats.TestingGetSum(load.Requests)
-			writesAfter := repl.loadStats.TestingGetSum(load.WriteKeys)
-			readsAfter := repl.loadStats.TestingGetSum(load.ReadKeys)
-			readBytesAfter := repl.loadStats.TestingGetSum(load.ReadBytes)
-			writeBytesAfter := repl.loadStats.TestingGetSum(load.WriteBytes)
-
-			assertGreaterThanInDelta(t, testCase.expectedRQPS, requestsAfter, epsilonAllowed)
-			assertGreaterThanInDelta(t, testCase.expectedWPS, writesAfter, epsilonAllowed)
-			assertGreaterThanInDelta(t, testCase.expectedRPS, readsAfter, epsilonAllowed)
-			assertGreaterThanInDelta(t, testCase.expectedRBPS, readBytesAfter, epsilonAllowed)
-			// NB: We assert that the written bytes is greater than the write
-			// batch request size. However the size multiplication factor,
-			// varies between 3 and 5 so we instead assert that it is greater
-			// than the logical bytes.
-			require.GreaterOrEqual(t, writeBytesAfter, testCase.expectedWBPS)
-			return nil
-		})
+		// Reset the request counts to 0 before sending to clear previous requests.
+		repl.loadStats.Reset()
+
+		requestsBefore := repl.loadStats.TestingGetSum(load.Requests)
+		writesBefore := repl.loadStats.TestingGetSum(load.WriteKeys)
+		readsBefore := repl.loadStats.TestingGetSum(load.ReadKeys)
+		readBytesBefore := repl.loadStats.TestingGetSum(load.ReadBytes)
+		writeBytesBefore := repl.loadStats.TestingGetSum(load.WriteBytes)
+
+		for i := 0; i < testCase.writes; i++ {
+			_, pErr := db.Inc(ctx, scratchKey, 1)
+			require.Nil(t, pErr)
+		}
+		require.Equal(t, 0.0, requestsBefore)
+		require.Equal(t, 0.0, writesBefore)
+		require.Equal(t, 0.0, readsBefore)
+		require.Equal(t, 0.0, writeBytesBefore)
+		require.Equal(t, 0.0, readBytesBefore)
+
+		requestsAfter := repl.loadStats.TestingGetSum(load.Requests)
+		writesAfter := repl.loadStats.TestingGetSum(load.WriteKeys)
+		readsAfter := repl.loadStats.TestingGetSum(load.ReadKeys)
+		readBytesAfter := repl.loadStats.TestingGetSum(load.ReadBytes)
+		writeBytesAfter := repl.loadStats.TestingGetSum(load.WriteBytes)
+
+		assertGreaterThanInDelta(t, testCase.expectedRQPS, requestsAfter, epsilonAllowed)
+		assertGreaterThanInDelta(t, testCase.expectedWPS, writesAfter, epsilonAllowed)
+		assertGreaterThanInDelta(t, testCase.expectedRPS, readsAfter, epsilonAllowed)
+		assertGreaterThanInDelta(t, testCase.expectedRBPS, readBytesAfter, epsilonAllowed)
+		// NB: We assert that the written bytes is greater than the write
+		// batch request size. However the size multiplication factor,
+		// varies between 3 and 5 so we instead assert that it is greater
+		// than the logical bytes.
+		require.GreaterOrEqual(t, writeBytesAfter, testCase.expectedWBPS)
 	}
 }