Merge pull request cockroachdb#8787 from RaduBerinde/scanner-test-fol…

…lowup

storage: better fix for TestScannerDisabled

storage/scanner.go

@@ -72,8 +72,9 @@ type replicaScanner struct {
 	// Count of times and total duration through the scanning loop.
 	mu struct {
 		syncutil.Mutex
-		count int64
-		total time.Duration
+		scanCount        int64
+		waitEnabledCount int64
+		total            time.Duration
 		// Some tests in this package disable scanning.
 		disabled bool
 	}
@@ -116,12 +117,20 @@ func (rs *replicaScanner) Start(clock *hlc.Clock, stopper *stop.Stopper) {
 	rs.scanLoop(clock, stopper)
 }
 
-// Count returns the number of times the scanner has cycled through
+// scanCount returns the number of times the scanner has cycled through
 // all replicas.
-func (rs *replicaScanner) Count() int64 {
+func (rs *replicaScanner) scanCount() int64 {
 	rs.mu.Lock()
 	defer rs.mu.Unlock()
-	return rs.mu.count
+	return rs.mu.scanCount
+}
+
+// waitEnabledCount returns the number of times the scanner went in the mode of
+// waiting to be reenabled.
+func (rs *replicaScanner) waitEnabledCount() int64 {
+	rs.mu.Lock()
+	defer rs.mu.Unlock()
+	return rs.mu.waitEnabledCount
 }
 
 // SetDisabled turns replica scanning off or on as directed. Note that while
@@ -147,7 +156,7 @@ func (rs *replicaScanner) GetDisabled() bool {
 func (rs *replicaScanner) avgScan() time.Duration {
 	rs.mu.Lock()
 	defer rs.mu.Unlock()
-	return time.Duration(rs.mu.total.Nanoseconds() / rs.mu.count)
+	return time.Duration(rs.mu.total.Nanoseconds() / rs.mu.scanCount)
 }
 
 // RemoveReplica removes a replica from any replica queues the scanner may
@@ -259,7 +268,7 @@ func (rs *replicaScanner) scanLoop(clock *hlc.Clock, stopper *stop.Stopper) {
 				// Increment iteration count.
 				rs.mu.Lock()
 				defer rs.mu.Unlock()
-				rs.mu.count++
+				rs.mu.scanCount++
 				rs.mu.total += timeutil.Since(start)
 				if log.V(6) {
 					log.Infof(context.TODO(), "reset replica scan iteration")
@@ -278,6 +287,9 @@ func (rs *replicaScanner) scanLoop(clock *hlc.Clock, stopper *stop.Stopper) {
 // waitEnabled loops, removing replicas from the scanner's queues,
 // until scanning is enabled or the stopper signals shutdown,
 func (rs *replicaScanner) waitEnabled(stopper *stop.Stopper) bool {
+	rs.mu.Lock()
+	rs.mu.waitEnabledCount++
+	rs.mu.Unlock()
 	for {
 		if !rs.GetDisabled() {
 			return false

storage/scanner_test.go

@@ -306,10 +306,9 @@ func TestScannerPaceInterval(t *testing.T) {
 func TestScannerDisabled(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	const count = 3
-	const scanInterval = 1 * time.Millisecond
 	ranges := newTestRangeSet(count, t)
 	q := &testQueue{}
-	s := newReplicaScanner(scanInterval, 0, ranges)
+	s := newReplicaScanner(1*time.Millisecond, 0, ranges)
 	s.AddQueues(q)
 	mc := hlc.NewManualClock(0)
 	clock := hlc.NewClock(mc.UnixNano)
@@ -322,28 +321,25 @@ func TestScannerDisabled(t *testing.T) {
 		if q.count() != count {
 			return errors.Errorf("expected %d replicas; have %d", count, q.count())
 		}
-		if s.Count() == 0 {
+		if s.scanCount() == 0 {
 			return errors.Errorf("expected scanner count to increment")
 		}
 		return nil
 	})
 
+	lastWaitEnabledCount := s.waitEnabledCount()
+
 	// Now, disable the scanner.
 	s.SetDisabled(true)
-	lastScannerCount := s.Count()
 	util.SucceedsSoon(t, func() error {
-		// We want to make sure that a complete scanner loop has time to complete
-		// between every retry iteration, so twice the scan interval is a safe
-		// value. In addition, the internal timers used by the scanner can
-		// occasionally fire with delay, so we want to be extra patient (see #8709).
-		time.Sleep(2*scanInterval + 200*time.Millisecond)
-		if sc := s.Count(); sc != lastScannerCount {
-			lastScannerCount = sc
+		if s.waitEnabledCount() == lastWaitEnabledCount {
 			return errors.Errorf("expected scanner to stop when disabled")
 		}
 		return nil
 	})
 
+	lastScannerCount := s.scanCount()
+
 	// Remove the replicas and verify the scanner still removes them while disabled.
 	ranges.Visit(func(repl *Replica) bool {
 		s.RemoveReplica(repl)
@@ -356,7 +352,7 @@ func TestScannerDisabled(t *testing.T) {
 		}
 		return nil
 	})
-	if sc := s.Count(); sc != lastScannerCount {
+	if sc := s.scanCount(); sc != lastScannerCount {
 		t.Errorf("expected scanner count to not increment: %d != %d", sc, lastScannerCount)
 	}
 }
@@ -383,7 +379,7 @@ func TestScannerEmptyRangeSet(t *testing.T) {
 	defer stopper.Stop()
 	s.Start(clock, stopper)
 	time.Sleep(time.Millisecond) // give it some time to (not) busy loop
-	if count := s.Count(); count > 1 {
+	if count := s.scanCount(); count > 1 {
 		t.Errorf("expected at most one loop, but got %d", count)
 	}
 }