From 9793dabcbb5c63e6bf75d51d007755b7b7e2e47c Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Thu, 19 Dec 2024 04:25:06 +0100 Subject: [PATCH 1/7] Avoid flakiness in topo concurrency test Signed-off-by: Tim Vaillancourt --- go/vt/topo/stats_conn_test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/go/vt/topo/stats_conn_test.go b/go/vt/topo/stats_conn_test.go index 9bc1d51d9ed..c6d37f6cdb3 100644 --- a/go/vt/topo/stats_conn_test.go +++ b/go/vt/topo/stats_conn_test.go @@ -185,11 +185,12 @@ func (st *fakeConn) IsReadOnly() bool { } // createTestReadSemaphoreContention simulates semaphore contention on the test read semaphore. -func createTestReadSemaphoreContention(ctx context.Context, duration time.Duration) { +func createTestReadSemaphoreContention(ctx context.Context, duration time.Duration, semAcquiredChan chan bool) { if err := testStatsConnReadSem.Acquire(ctx, 1); err != nil { panic(err) } defer testStatsConnReadSem.Release(1) + semAcquiredChan <- true time.Sleep(duration) } @@ -199,7 +200,9 @@ func TestStatsConnTopoListDir(t *testing.T) { statsConn := NewStatsConn("global", conn, testStatsConnReadSem) ctx := context.Background() - go createTestReadSemaphoreContention(ctx, 100*time.Millisecond) + semAcquired := make(chan bool) + go createTestReadSemaphoreContention(ctx, 100*time.Millisecond, semAcquired) + <-semAcquired statsConn.ListDir(ctx, "", true) timingCounts := topoStatsConnTimings.Counts()["ListDir.global"] if got, want := timingCounts, int64(1); got != want { @@ -286,7 +289,9 @@ func TestStatsConnTopoGet(t *testing.T) { statsConn := NewStatsConn("global", conn, testStatsConnReadSem) ctx := context.Background() - go createTestReadSemaphoreContention(ctx, time.Millisecond*100) + semAcquired := make(chan bool) + go createTestReadSemaphoreContention(ctx, time.Millisecond*100, semAcquired) + <-semAcquired statsConn.Get(ctx, "") timingCounts := topoStatsConnTimings.Counts()["Get.global"] if got, want := timingCounts, int64(1); got != want { From c1f3291866b47721c196bf477d1bb4d8f84eb944 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Thu, 19 Dec 2024 06:33:11 +0100 Subject: [PATCH 2/7] modernize test Signed-off-by: Tim Vaillancourt --- go/vt/topo/stats_conn_test.go | 188 +++++++++++++++------------------- 1 file changed, 83 insertions(+), 105 deletions(-) diff --git a/go/vt/topo/stats_conn_test.go b/go/vt/topo/stats_conn_test.go index c6d37f6cdb3..e0fe055e9d3 100644 --- a/go/vt/topo/stats_conn_test.go +++ b/go/vt/topo/stats_conn_test.go @@ -196,232 +196,210 @@ func createTestReadSemaphoreContention(ctx context.Context, duration time.Durati // TestStatsConnTopoListDir emits stats on ListDir func TestStatsConnTopoListDir(t *testing.T) { + defer func() { + topoStatsConnTimings.Reset() + topoStatsConnReadWaitTimings.Reset() + }() + conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) ctx := context.Background() - semAcquired := make(chan bool) - go createTestReadSemaphoreContention(ctx, 100*time.Millisecond, semAcquired) - <-semAcquired + semAcquiredChan := make(chan bool, 0 /* no buffer */) + go createTestReadSemaphoreContention(ctx, 100*time.Millisecond, semAcquiredChan) + <-semAcquiredChan statsConn.ListDir(ctx, "", true) - timingCounts := topoStatsConnTimings.Counts()["ListDir.global"] - if got, want := timingCounts, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnTimings.Counts()["ListDir.global"]) + require.NotZero(t, topoStatsConnTimings.Time()) - waitTimingsCounts := topoStatsConnReadWaitTimings.Counts()["ListDir.global"] - if got := waitTimingsCounts; got != 1 { - t.Errorf("stats were not properly recorded: got = %d, want = 1", got) - } + require.Equal(t, int64(1), topoStatsConnReadWaitTimings.Counts()["ListDir.global"]) + require.NotZero(t, topoStatsConnReadWaitTimings.Time()) // error is zero before getting an error - errorCount := topoStatsConnErrors.Counts()["ListDir.global"] - if got, want := errorCount, int64(0); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Zero(t, topoStatsConnErrors.Counts()["ListDir.global"]) statsConn.ListDir(ctx, "error", true) // error stats gets emitted - errorCount = topoStatsConnErrors.Counts()["ListDir.global"] - if got, want := errorCount, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnErrors.Counts()["ListDir.global"]) } // TestStatsConnTopoCreate emits stats on Create func TestStatsConnTopoCreate(t *testing.T) { + defer func() { + topoStatsConnTimings.Reset() + topoStatsConnReadWaitTimings.Reset() + }() + conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) ctx := context.Background() statsConn.Create(ctx, "", []byte{}) - timingCounts := topoStatsConnTimings.Counts()["Create.global"] - if got, want := timingCounts, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Create.global"]) + require.NotZero(t, topoStatsConnTimings.Time()) // error is zero before getting an error - errorCount := topoStatsConnErrors.Counts()["Create.global"] - if got, want := errorCount, int64(0); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Zero(t, topoStatsConnErrors.Counts()["Create.global"]) statsConn.Create(ctx, "error", []byte{}) // error stats gets emitted - errorCount = topoStatsConnErrors.Counts()["Create.global"] - if got, want := errorCount, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnErrors.Counts()["Create.global"]) } // TestStatsConnTopoUpdate emits stats on Update func TestStatsConnTopoUpdate(t *testing.T) { + defer func() { + topoStatsConnTimings.Reset() + topoStatsConnReadWaitTimings.Reset() + }() + conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) ctx := context.Background() statsConn.Update(ctx, "", []byte{}, conn.v) - timingCounts := topoStatsConnTimings.Counts()["Update.global"] - if got, want := timingCounts, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Update.global"]) + require.NotZero(t, topoStatsConnTimings.Time()) // error is zero before getting an error - errorCount := topoStatsConnErrors.Counts()["Update.global"] - if got, want := errorCount, int64(0); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Zero(t, topoStatsConnErrors.Counts()["Update.global"]) statsConn.Update(ctx, "error", []byte{}, conn.v) // error stats gets emitted - errorCount = topoStatsConnErrors.Counts()["Update.global"] - if got, want := errorCount, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnErrors.Counts()["Update.global"]) } // TestStatsConnTopoGet emits stats on Get func TestStatsConnTopoGet(t *testing.T) { + defer func() { + topoStatsConnTimings.Reset() + topoStatsConnReadWaitTimings.Reset() + }() + conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) ctx := context.Background() - semAcquired := make(chan bool) - go createTestReadSemaphoreContention(ctx, time.Millisecond*100, semAcquired) - <-semAcquired + semAcquiredChan := make(chan bool, 0 /* no buffer */) + go createTestReadSemaphoreContention(ctx, time.Millisecond*100, semAcquiredChan) + <-semAcquiredChan statsConn.Get(ctx, "") - timingCounts := topoStatsConnTimings.Counts()["Get.global"] - if got, want := timingCounts, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Get.global"]) + require.NotZero(t, topoStatsConnTimings.Time()) - waitTimingsCounts := topoStatsConnReadWaitTimings.Counts()["Get.global"] - if got := waitTimingsCounts; got != 1 { - t.Errorf("stats were not properly recorded: got = %d, want = 1", got) - } + require.Equal(t, int64(1), topoStatsConnReadWaitTimings.Counts()["Get.global"]) + require.NotZero(t, topoStatsConnReadWaitTimings.Time()) // error is zero before getting an error - errorCount := topoStatsConnErrors.Counts()["Get.global"] - if got, want := errorCount, int64(0); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Zero(t, topoStatsConnErrors.Counts()["Get.global"]) statsConn.Get(ctx, "error") // error stats gets emitted - errorCount = topoStatsConnErrors.Counts()["Get.global"] - if got, want := errorCount, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnErrors.Counts()["Get.global"]) } // TestStatsConnTopoDelete emits stats on Delete func TestStatsConnTopoDelete(t *testing.T) { + defer func() { + topoStatsConnTimings.Reset() + topoStatsConnReadWaitTimings.Reset() + }() + conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) ctx := context.Background() statsConn.Delete(ctx, "", conn.v) - timingCounts := topoStatsConnTimings.Counts()["Delete.global"] - if got, want := timingCounts, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Delete.global"]) + require.NotZero(t, topoStatsConnTimings.Time()) // error is zero before getting an error - errorCount := topoStatsConnErrors.Counts()["Delete.global"] - if got, want := errorCount, int64(0); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Zero(t, topoStatsConnErrors.Counts()["Delete.global"]) statsConn.Delete(ctx, "error", conn.v) // error stats gets emitted - errorCount = topoStatsConnErrors.Counts()["Delete.global"] - if got, want := errorCount, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnErrors.Counts()["Delete.global"]) } // TestStatsConnTopoLock emits stats on Lock func TestStatsConnTopoLock(t *testing.T) { + defer func() { + topoStatsConnTimings.Reset() + topoStatsConnReadWaitTimings.Reset() + }() + conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) ctx := context.Background() statsConn.Lock(ctx, "", "") - timingCounts := topoStatsConnTimings.Counts()["Lock.global"] - require.Equal(t, timingCounts, int64(1)) + require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Lock.global"]) + require.NotZero(t, topoStatsConnTimings.Time()) statsConn.LockWithTTL(ctx, "", "", time.Second) - timingCounts = topoStatsConnTimings.Counts()["LockWithTTL.global"] - require.Equal(t, timingCounts, int64(1)) + require.Equal(t, int64(1), topoStatsConnTimings.Counts()["LockWithTTL.global"]) statsConn.LockName(ctx, "", "") - timingCounts = topoStatsConnTimings.Counts()["LockName.global"] - require.Equal(t, timingCounts, int64(1)) + require.Equal(t, int64(1), topoStatsConnTimings.Counts()["LockName.global"]) // Error is zero before getting an error. - errorCount := topoStatsConnErrors.Counts()["Lock.global"] - require.Equal(t, errorCount, int64(0)) + require.Zero(t, topoStatsConnErrors.Counts()["Lock.global"]) statsConn.Lock(ctx, "error", "") // Error stats gets emitted. - errorCount = topoStatsConnErrors.Counts()["Lock.global"] - require.Equal(t, errorCount, int64(1)) + require.Equal(t, int64(1), topoStatsConnErrors.Counts()["Lock.global"]) } // TestStatsConnTopoWatch emits stats on Watch func TestStatsConnTopoWatch(t *testing.T) { + defer topoStatsConnTimings.Reset() + conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) ctx := context.Background() statsConn.Watch(ctx, "") - timingCounts := topoStatsConnTimings.Counts()["Watch.global"] - if got, want := timingCounts, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } - + require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Watch.global"]) + require.NotZero(t, topoStatsConnTimings.Time()) } // TestStatsConnTopoNewLeaderParticipation emits stats on NewLeaderParticipation func TestStatsConnTopoNewLeaderParticipation(t *testing.T) { + defer func() { + topoStatsConnTimings.Reset() + topoStatsConnReadWaitTimings.Reset() + }() + conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) _, _ = statsConn.NewLeaderParticipation("", "") - timingCounts := topoStatsConnTimings.Counts()["NewLeaderParticipation.global"] - if got, want := timingCounts, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnTimings.Counts()["NewLeaderParticipation.global"]) + require.NotZero(t, topoStatsConnTimings.Time()) // error is zero before getting an error - errorCount := topoStatsConnErrors.Counts()["NewLeaderParticipation.global"] - if got, want := errorCount, int64(0); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Zero(t, topoStatsConnErrors.Counts()["NewLeaderParticipation.global"]) _, _ = statsConn.NewLeaderParticipation("error", "") // error stats gets emitted - errorCount = topoStatsConnErrors.Counts()["NewLeaderParticipation.global"] - if got, want := errorCount, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnErrors.Counts()["NewLeaderParticipation.global"]) } // TestStatsConnTopoClose emits stats on Close func TestStatsConnTopoClose(t *testing.T) { + defer topoStatsConnTimings.Reset() + conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) statsConn.Close() - timingCounts := topoStatsConnTimings.Counts()["Close.global"] - if got, want := timingCounts, int64(1); got != want { - t.Errorf("stats were not properly recorded: got = %d, want = %d", got, want) - } + require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Close.global"]) + require.NotZero(t, topoStatsConnTimings.Time()) } From bd2a0873278492bad4792146b9aeac38677fc68c Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Thu, 19 Dec 2024 06:48:21 +0100 Subject: [PATCH 3/7] more cleanup Signed-off-by: Tim Vaillancourt --- go/vt/topo/stats_conn_test.go | 52 +++++++++++++++-------------------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/go/vt/topo/stats_conn_test.go b/go/vt/topo/stats_conn_test.go index e0fe055e9d3..631688d210a 100644 --- a/go/vt/topo/stats_conn_test.go +++ b/go/vt/topo/stats_conn_test.go @@ -31,6 +31,12 @@ import ( var testStatsConnReadSem = semaphore.NewWeighted(1) +func testResetStats() { + topoStatsConnErrors.ResetAll() + topoStatsConnTimings.Reset() + topoStatsConnReadWaitTimings.Reset() +} + // The fakeConn is a wrapper for a Conn that emits stats for every operation type fakeConn struct { v Version @@ -196,10 +202,7 @@ func createTestReadSemaphoreContention(ctx context.Context, duration time.Durati // TestStatsConnTopoListDir emits stats on ListDir func TestStatsConnTopoListDir(t *testing.T) { - defer func() { - topoStatsConnTimings.Reset() - topoStatsConnReadWaitTimings.Reset() - }() + defer testResetStats() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -226,10 +229,7 @@ func TestStatsConnTopoListDir(t *testing.T) { // TestStatsConnTopoCreate emits stats on Create func TestStatsConnTopoCreate(t *testing.T) { - defer func() { - topoStatsConnTimings.Reset() - topoStatsConnReadWaitTimings.Reset() - }() + defer testResetStats() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -238,6 +238,7 @@ func TestStatsConnTopoCreate(t *testing.T) { statsConn.Create(ctx, "", []byte{}) require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Create.global"]) require.NotZero(t, topoStatsConnTimings.Time()) + require.Zero(t, topoStatsConnReadWaitTimings.Time()) // error is zero before getting an error require.Zero(t, topoStatsConnErrors.Counts()["Create.global"]) @@ -250,10 +251,7 @@ func TestStatsConnTopoCreate(t *testing.T) { // TestStatsConnTopoUpdate emits stats on Update func TestStatsConnTopoUpdate(t *testing.T) { - defer func() { - topoStatsConnTimings.Reset() - topoStatsConnReadWaitTimings.Reset() - }() + defer testResetStats() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -262,6 +260,7 @@ func TestStatsConnTopoUpdate(t *testing.T) { statsConn.Update(ctx, "", []byte{}, conn.v) require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Update.global"]) require.NotZero(t, topoStatsConnTimings.Time()) + require.Zero(t, topoStatsConnReadWaitTimings.Time()) // error is zero before getting an error require.Zero(t, topoStatsConnErrors.Counts()["Update.global"]) @@ -274,10 +273,7 @@ func TestStatsConnTopoUpdate(t *testing.T) { // TestStatsConnTopoGet emits stats on Get func TestStatsConnTopoGet(t *testing.T) { - defer func() { - topoStatsConnTimings.Reset() - topoStatsConnReadWaitTimings.Reset() - }() + defer testResetStats() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -304,10 +300,7 @@ func TestStatsConnTopoGet(t *testing.T) { // TestStatsConnTopoDelete emits stats on Delete func TestStatsConnTopoDelete(t *testing.T) { - defer func() { - topoStatsConnTimings.Reset() - topoStatsConnReadWaitTimings.Reset() - }() + defer testResetStats() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -316,6 +309,7 @@ func TestStatsConnTopoDelete(t *testing.T) { statsConn.Delete(ctx, "", conn.v) require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Delete.global"]) require.NotZero(t, topoStatsConnTimings.Time()) + require.Zero(t, topoStatsConnReadWaitTimings.Time()) // error is zero before getting an error require.Zero(t, topoStatsConnErrors.Counts()["Delete.global"]) @@ -328,10 +322,7 @@ func TestStatsConnTopoDelete(t *testing.T) { // TestStatsConnTopoLock emits stats on Lock func TestStatsConnTopoLock(t *testing.T) { - defer func() { - topoStatsConnTimings.Reset() - topoStatsConnReadWaitTimings.Reset() - }() + defer testResetStats() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -340,6 +331,7 @@ func TestStatsConnTopoLock(t *testing.T) { statsConn.Lock(ctx, "", "") require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Lock.global"]) require.NotZero(t, topoStatsConnTimings.Time()) + require.Zero(t, topoStatsConnReadWaitTimings.Time()) statsConn.LockWithTTL(ctx, "", "", time.Second) require.Equal(t, int64(1), topoStatsConnTimings.Counts()["LockWithTTL.global"]) @@ -358,7 +350,7 @@ func TestStatsConnTopoLock(t *testing.T) { // TestStatsConnTopoWatch emits stats on Watch func TestStatsConnTopoWatch(t *testing.T) { - defer topoStatsConnTimings.Reset() + defer testResetStats() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -367,14 +359,12 @@ func TestStatsConnTopoWatch(t *testing.T) { statsConn.Watch(ctx, "") require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Watch.global"]) require.NotZero(t, topoStatsConnTimings.Time()) + require.Zero(t, topoStatsConnReadWaitTimings.Time()) } // TestStatsConnTopoNewLeaderParticipation emits stats on NewLeaderParticipation func TestStatsConnTopoNewLeaderParticipation(t *testing.T) { - defer func() { - topoStatsConnTimings.Reset() - topoStatsConnReadWaitTimings.Reset() - }() + defer testResetStats() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -382,6 +372,7 @@ func TestStatsConnTopoNewLeaderParticipation(t *testing.T) { _, _ = statsConn.NewLeaderParticipation("", "") require.Equal(t, int64(1), topoStatsConnTimings.Counts()["NewLeaderParticipation.global"]) require.NotZero(t, topoStatsConnTimings.Time()) + require.Zero(t, topoStatsConnReadWaitTimings.Time()) // error is zero before getting an error require.Zero(t, topoStatsConnErrors.Counts()["NewLeaderParticipation.global"]) @@ -394,7 +385,7 @@ func TestStatsConnTopoNewLeaderParticipation(t *testing.T) { // TestStatsConnTopoClose emits stats on Close func TestStatsConnTopoClose(t *testing.T) { - defer topoStatsConnTimings.Reset() + defer testResetStats() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -402,4 +393,5 @@ func TestStatsConnTopoClose(t *testing.T) { statsConn.Close() require.Equal(t, int64(1), topoStatsConnTimings.Counts()["Close.global"]) require.NotZero(t, topoStatsConnTimings.Time()) + require.Zero(t, topoStatsConnReadWaitTimings.Time()) } From 895fcb510ebab44bd8ab25658c76958199696e9f Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Thu, 19 Dec 2024 06:49:20 +0100 Subject: [PATCH 4/7] var rename Signed-off-by: Tim Vaillancourt --- go/vt/topo/stats_conn_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/go/vt/topo/stats_conn_test.go b/go/vt/topo/stats_conn_test.go index 631688d210a..2ed280630a7 100644 --- a/go/vt/topo/stats_conn_test.go +++ b/go/vt/topo/stats_conn_test.go @@ -31,7 +31,7 @@ import ( var testStatsConnReadSem = semaphore.NewWeighted(1) -func testResetStats() { +func testStatsConnStatsReset() { topoStatsConnErrors.ResetAll() topoStatsConnTimings.Reset() topoStatsConnReadWaitTimings.Reset() @@ -202,7 +202,7 @@ func createTestReadSemaphoreContention(ctx context.Context, duration time.Durati // TestStatsConnTopoListDir emits stats on ListDir func TestStatsConnTopoListDir(t *testing.T) { - defer testResetStats() + defer testStatsConnStatsReset() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -229,7 +229,7 @@ func TestStatsConnTopoListDir(t *testing.T) { // TestStatsConnTopoCreate emits stats on Create func TestStatsConnTopoCreate(t *testing.T) { - defer testResetStats() + defer testStatsConnStatsReset() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -251,7 +251,7 @@ func TestStatsConnTopoCreate(t *testing.T) { // TestStatsConnTopoUpdate emits stats on Update func TestStatsConnTopoUpdate(t *testing.T) { - defer testResetStats() + defer testStatsConnStatsReset() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -273,7 +273,7 @@ func TestStatsConnTopoUpdate(t *testing.T) { // TestStatsConnTopoGet emits stats on Get func TestStatsConnTopoGet(t *testing.T) { - defer testResetStats() + defer testStatsConnStatsReset() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -300,7 +300,7 @@ func TestStatsConnTopoGet(t *testing.T) { // TestStatsConnTopoDelete emits stats on Delete func TestStatsConnTopoDelete(t *testing.T) { - defer testResetStats() + defer testStatsConnStatsReset() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -322,7 +322,7 @@ func TestStatsConnTopoDelete(t *testing.T) { // TestStatsConnTopoLock emits stats on Lock func TestStatsConnTopoLock(t *testing.T) { - defer testResetStats() + defer testStatsConnStatsReset() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -350,7 +350,7 @@ func TestStatsConnTopoLock(t *testing.T) { // TestStatsConnTopoWatch emits stats on Watch func TestStatsConnTopoWatch(t *testing.T) { - defer testResetStats() + defer testStatsConnStatsReset() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -364,7 +364,7 @@ func TestStatsConnTopoWatch(t *testing.T) { // TestStatsConnTopoNewLeaderParticipation emits stats on NewLeaderParticipation func TestStatsConnTopoNewLeaderParticipation(t *testing.T) { - defer testResetStats() + defer testStatsConnStatsReset() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) @@ -385,7 +385,7 @@ func TestStatsConnTopoNewLeaderParticipation(t *testing.T) { // TestStatsConnTopoClose emits stats on Close func TestStatsConnTopoClose(t *testing.T) { - defer testResetStats() + defer testStatsConnStatsReset() conn := &fakeConn{} statsConn := NewStatsConn("global", conn, testStatsConnReadSem) From 4736966db891a354d53b54c981c72ad57b4b208a Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Thu, 19 Dec 2024 06:53:07 +0100 Subject: [PATCH 5/7] more cleanup Signed-off-by: Tim Vaillancourt --- go/vt/topo/stats_conn_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/go/vt/topo/stats_conn_test.go b/go/vt/topo/stats_conn_test.go index 2ed280630a7..bbc7ee311f8 100644 --- a/go/vt/topo/stats_conn_test.go +++ b/go/vt/topo/stats_conn_test.go @@ -202,6 +202,7 @@ func createTestReadSemaphoreContention(ctx context.Context, duration time.Durati // TestStatsConnTopoListDir emits stats on ListDir func TestStatsConnTopoListDir(t *testing.T) { + testStatsConnStatsReset() defer testStatsConnStatsReset() conn := &fakeConn{} @@ -229,6 +230,7 @@ func TestStatsConnTopoListDir(t *testing.T) { // TestStatsConnTopoCreate emits stats on Create func TestStatsConnTopoCreate(t *testing.T) { + testStatsConnStatsReset() defer testStatsConnStatsReset() conn := &fakeConn{} @@ -251,6 +253,7 @@ func TestStatsConnTopoCreate(t *testing.T) { // TestStatsConnTopoUpdate emits stats on Update func TestStatsConnTopoUpdate(t *testing.T) { + testStatsConnStatsReset() defer testStatsConnStatsReset() conn := &fakeConn{} @@ -273,6 +276,7 @@ func TestStatsConnTopoUpdate(t *testing.T) { // TestStatsConnTopoGet emits stats on Get func TestStatsConnTopoGet(t *testing.T) { + testStatsConnStatsReset() defer testStatsConnStatsReset() conn := &fakeConn{} @@ -300,6 +304,7 @@ func TestStatsConnTopoGet(t *testing.T) { // TestStatsConnTopoDelete emits stats on Delete func TestStatsConnTopoDelete(t *testing.T) { + testStatsConnStatsReset() defer testStatsConnStatsReset() conn := &fakeConn{} @@ -322,6 +327,7 @@ func TestStatsConnTopoDelete(t *testing.T) { // TestStatsConnTopoLock emits stats on Lock func TestStatsConnTopoLock(t *testing.T) { + testStatsConnStatsReset() defer testStatsConnStatsReset() conn := &fakeConn{} @@ -350,6 +356,7 @@ func TestStatsConnTopoLock(t *testing.T) { // TestStatsConnTopoWatch emits stats on Watch func TestStatsConnTopoWatch(t *testing.T) { + testStatsConnStatsReset() defer testStatsConnStatsReset() conn := &fakeConn{} @@ -364,6 +371,7 @@ func TestStatsConnTopoWatch(t *testing.T) { // TestStatsConnTopoNewLeaderParticipation emits stats on NewLeaderParticipation func TestStatsConnTopoNewLeaderParticipation(t *testing.T) { + testStatsConnStatsReset() defer testStatsConnStatsReset() conn := &fakeConn{} @@ -385,6 +393,7 @@ func TestStatsConnTopoNewLeaderParticipation(t *testing.T) { // TestStatsConnTopoClose emits stats on Close func TestStatsConnTopoClose(t *testing.T) { + testStatsConnStatsReset() defer testStatsConnStatsReset() conn := &fakeConn{} From 82cd7a115d659bbfc7e02044bca27b3668800b9c Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Thu, 19 Dec 2024 07:03:34 +0100 Subject: [PATCH 6/7] lint Signed-off-by: Tim Vaillancourt --- go/vt/topo/stats_conn_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/vt/topo/stats_conn_test.go b/go/vt/topo/stats_conn_test.go index bbc7ee311f8..31b7cb36072 100644 --- a/go/vt/topo/stats_conn_test.go +++ b/go/vt/topo/stats_conn_test.go @@ -209,7 +209,7 @@ func TestStatsConnTopoListDir(t *testing.T) { statsConn := NewStatsConn("global", conn, testStatsConnReadSem) ctx := context.Background() - semAcquiredChan := make(chan bool, 0 /* no buffer */) + semAcquiredChan := make(chan bool) go createTestReadSemaphoreContention(ctx, 100*time.Millisecond, semAcquiredChan) <-semAcquiredChan statsConn.ListDir(ctx, "", true) @@ -283,7 +283,7 @@ func TestStatsConnTopoGet(t *testing.T) { statsConn := NewStatsConn("global", conn, testStatsConnReadSem) ctx := context.Background() - semAcquiredChan := make(chan bool, 0 /* no buffer */) + semAcquiredChan := make(chan bool) go createTestReadSemaphoreContention(ctx, time.Millisecond*100, semAcquiredChan) <-semAcquiredChan statsConn.Get(ctx, "") From 37eb29abe0a5486ff5bcdbfd3934d9b72658544f Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Thu, 19 Dec 2024 07:08:05 +0100 Subject: [PATCH 7/7] better comments Signed-off-by: Tim Vaillancourt --- go/vt/topo/stats_conn_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/go/vt/topo/stats_conn_test.go b/go/vt/topo/stats_conn_test.go index 31b7cb36072..d58f5ab6918 100644 --- a/go/vt/topo/stats_conn_test.go +++ b/go/vt/topo/stats_conn_test.go @@ -29,12 +29,16 @@ import ( "vitess.io/vitess/go/vt/vterrors" ) +// testStatsConnReadSem is a semaphore for unit tests. +// It intentionally has a concurrency limit of '1' to +// allow semaphore contention in tests. var testStatsConnReadSem = semaphore.NewWeighted(1) +// testStatsConnStatsReset resets StatsConn-based stats. func testStatsConnStatsReset() { topoStatsConnErrors.ResetAll() - topoStatsConnTimings.Reset() topoStatsConnReadWaitTimings.Reset() + topoStatsConnTimings.Reset() } // The fakeConn is a wrapper for a Conn that emits stats for every operation