From 2cb42d1f919a4eaec895b33b3a18ddb12c430f57 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sat, 24 Feb 2024 00:52:47 +0100 Subject: [PATCH 01/10] Filter by keyspace earlier in `tabletgateway`s `WaitForTablets(...)` Signed-off-by: Tim Vaillancourt --- go/vt/discovery/healthcheck.go | 22 ------------- go/vt/discovery/healthcheck_test.go | 7 ----- go/vt/srvtopo/discover.go | 19 +++++++++++- go/vt/srvtopo/discover_test.go | 48 ++++++++++++++++++++++++++--- go/vt/vtgate/tabletgateway.go | 2 +- 5 files changed, 62 insertions(+), 36 deletions(-) diff --git a/go/vt/discovery/healthcheck.go b/go/vt/discovery/healthcheck.go index 5d6a5e32662..6b30a794893 100644 --- a/go/vt/discovery/healthcheck.go +++ b/go/vt/discovery/healthcheck.go @@ -750,30 +750,8 @@ func (hc *HealthCheckImpl) WaitForAllServingTablets(ctx context.Context, targets return hc.waitForTablets(ctx, targets, true) } -// FilterTargetsByKeyspaces only returns the targets that are part of the provided keyspaces -func FilterTargetsByKeyspaces(keyspaces []string, targets []*query.Target) []*query.Target { - filteredTargets := make([]*query.Target, 0) - - // Keep them all if there are no keyspaces to watch - if len(KeyspacesToWatch) == 0 { - return append(filteredTargets, targets...) - } - - // Let's remove from the target shards that are not in the keyspaceToWatch list. - for _, target := range targets { - for _, keyspaceToWatch := range keyspaces { - if target.Keyspace == keyspaceToWatch { - filteredTargets = append(filteredTargets, target) - } - } - } - return filteredTargets -} - // waitForTablets is the internal method that polls for tablets. func (hc *HealthCheckImpl) waitForTablets(ctx context.Context, targets []*query.Target, requireServing bool) error { - targets = FilterTargetsByKeyspaces(KeyspacesToWatch, targets) - for { // We nil targets as we find them. allPresent := true diff --git a/go/vt/discovery/healthcheck_test.go b/go/vt/discovery/healthcheck_test.go index 9563d9bfdc5..adc415cc497 100644 --- a/go/vt/discovery/healthcheck_test.go +++ b/go/vt/discovery/healthcheck_test.go @@ -686,13 +686,6 @@ func TestWaitForAllServingTablets(t *testing.T) { TabletType: tablet.Type, }, } - - KeyspacesToWatch = []string{tablet.Keyspace} - - err = hc.WaitForAllServingTablets(ctx, targets) - assert.Nil(t, err, "error should be nil. Keyspace with no tablets is filtered") - - KeyspacesToWatch = []string{} } // TestRemoveTablet tests the behavior when a tablet goes away. diff --git a/go/vt/srvtopo/discover.go b/go/vt/srvtopo/discover.go index 91aaea9daf6..7c7e4c58023 100644 --- a/go/vt/srvtopo/discover.go +++ b/go/vt/srvtopo/discover.go @@ -29,10 +29,24 @@ import ( topodatapb "vitess.io/vitess/go/vt/proto/topodata" ) +// shouldFindKeyspace returns true if a keyspace should be fetched. +// true is also returned if no keyspaces are defined. +func shouldFindKeyspace(keyspaces []string, ksName string) bool { + if len(keyspaces) == 0 { + return true + } + for _, keyspace := range keyspaces { + if ksName == keyspace { + return true + } + } + return false +} + // FindAllTargets goes through all serving shards in the topology // for the provided tablet types. It returns one Target object per // keyspace / shard / matching TabletType. -func FindAllTargets(ctx context.Context, ts Server, cell string, tabletTypes []topodatapb.TabletType) ([]*querypb.Target, error) { +func FindAllTargets(ctx context.Context, ts Server, cell string, keyspaces []string, tabletTypes []topodatapb.TabletType) ([]*querypb.Target, error) { ksNames, err := ts.GetSrvKeyspaceNames(ctx, cell, true) if err != nil { return nil, err @@ -43,6 +57,9 @@ func FindAllTargets(ctx context.Context, ts Server, cell string, tabletTypes []t var mu sync.Mutex var errRecorder concurrency.AllErrorRecorder for _, ksName := range ksNames { + if !shouldFindKeyspace(keyspaces, ksName) { + continue + } wg.Add(1) go func(keyspace string) { defer wg.Done() diff --git a/go/vt/srvtopo/discover_test.go b/go/vt/srvtopo/discover_test.go index 81279b7d61a..fbfde18e2e3 100644 --- a/go/vt/srvtopo/discover_test.go +++ b/go/vt/srvtopo/discover_test.go @@ -64,7 +64,7 @@ func TestFindAllTargets(t *testing.T) { rs := NewResilientServer(ctx, ts, counts) // No keyspace / shards. - ks, err := FindAllTargets(ctx, rs, "cell1", []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) + ks, err := FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) if err != nil { t.Errorf("unexpected error: %v", err) } @@ -89,7 +89,23 @@ func TestFindAllTargets(t *testing.T) { } // Get it. - ks, err = FindAllTargets(ctx, rs, "cell1", []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) + ks, err = FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if !reflect.DeepEqual(ks, []*querypb.Target{ + { + Cell: "cell1", + Keyspace: "test_keyspace", + Shard: "test_shard0", + TabletType: topodatapb.TabletType_PRIMARY, + }, + }) { + t.Errorf("got wrong value: %v", ks) + } + + // Get any keyspace. + ks, err = FindAllTargets(ctx, rs, "cell1", []string{}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) if err != nil { t.Errorf("unexpected error: %v", err) } @@ -129,7 +145,7 @@ func TestFindAllTargets(t *testing.T) { } // Get it for all types. - ks, err = FindAllTargets(ctx, rs, "cell1", []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) + ks, err = FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace", "test_keyspace2"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) if err != nil { t.Errorf("unexpected error: %v", err) } @@ -157,8 +173,30 @@ func TestFindAllTargets(t *testing.T) { t.Errorf("got wrong value: %v", ks) } - // Only get the REPLICA targets. - ks, err = FindAllTargets(ctx, rs, "cell1", []topodatapb.TabletType{topodatapb.TabletType_REPLICA}) + // Only get 1 keyspaces only for all types. + ks, err = FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace2"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if !reflect.DeepEqual(ks, []*querypb.Target{ + { + Cell: "cell1", + Keyspace: "test_keyspace2", + Shard: "test_shard1", + TabletType: topodatapb.TabletType_PRIMARY, + }, + { + Cell: "cell1", + Keyspace: "test_keyspace2", + Shard: "test_shard2", + TabletType: topodatapb.TabletType_REPLICA, + }, + }) { + t.Errorf("got wrong value: %v", ks) + } + + // Only get the REPLICA targets for any keyspace. + ks, err = FindAllTargets(ctx, rs, "cell1", []string{}, []topodatapb.TabletType{topodatapb.TabletType_REPLICA}) if err != nil { t.Errorf("unexpected error: %v", err) } diff --git a/go/vt/vtgate/tabletgateway.go b/go/vt/vtgate/tabletgateway.go index b289cf3b74e..49b9dac1128 100644 --- a/go/vt/vtgate/tabletgateway.go +++ b/go/vt/vtgate/tabletgateway.go @@ -191,7 +191,7 @@ func (gw *TabletGateway) WaitForTablets(ctx context.Context, tabletTypesToWait [ } // Finds the targets to look for. - targets, err := srvtopo.FindAllTargets(ctx, gw.srvTopoServer, gw.localCell, tabletTypesToWait) + targets, err := srvtopo.FindAllTargets(ctx, gw.srvTopoServer, gw.localCell, discovery.KeyspacesToWatch, tabletTypesToWait) if err != nil { return err } From e98a1b1a0f3c2104d51dddde6c7a43ba5ce935c2 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sat, 24 Feb 2024 01:12:44 +0100 Subject: [PATCH 02/10] golangci-lint Signed-off-by: Tim Vaillancourt --- go/vt/discovery/healthcheck_test.go | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/go/vt/discovery/healthcheck_test.go b/go/vt/discovery/healthcheck_test.go index adc415cc497..31376bd8c7d 100644 --- a/go/vt/discovery/healthcheck_test.go +++ b/go/vt/discovery/healthcheck_test.go @@ -672,20 +672,6 @@ func TestWaitForAllServingTablets(t *testing.T) { err = hc.WaitForAllServingTablets(ctx, targets) assert.NotNil(t, err, "error should not be nil (there are no tablets on this keyspace") - - targets = []*querypb.Target{ - - { - Keyspace: tablet.Keyspace, - Shard: tablet.Shard, - TabletType: tablet.Type, - }, - { - Keyspace: "newkeyspace", - Shard: tablet.Shard, - TabletType: tablet.Type, - }, - } } // TestRemoveTablet tests the behavior when a tablet goes away. From 946a9781cec9c35dc08ddbd3a405f5d3c36d1a93 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sat, 24 Feb 2024 01:27:36 +0100 Subject: [PATCH 03/10] Add test Signed-off-by: Tim Vaillancourt --- go/vt/srvtopo/discover_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/go/vt/srvtopo/discover_test.go b/go/vt/srvtopo/discover_test.go index fbfde18e2e3..2a3106cd2b4 100644 --- a/go/vt/srvtopo/discover_test.go +++ b/go/vt/srvtopo/discover_test.go @@ -210,4 +210,13 @@ func TestFindAllTargets(t *testing.T) { }) { t.Errorf("got wrong value: %v", ks) } + + // Get non-existent keyspace. + ks, err = FindAllTargets(ctx, rs, "cell1", []string{"doesnt-exist"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if len(ks) != 0 { + t.Errorf("got non-0 length value: %v", ks) + } } From 34b98e81e9007cc1483b15f859c3ee764b84997e Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sat, 24 Feb 2024 01:28:52 +0100 Subject: [PATCH 04/10] Fix typo Signed-off-by: Tim Vaillancourt --- go/vt/srvtopo/discover_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/srvtopo/discover_test.go b/go/vt/srvtopo/discover_test.go index 2a3106cd2b4..f1f35e1e43d 100644 --- a/go/vt/srvtopo/discover_test.go +++ b/go/vt/srvtopo/discover_test.go @@ -173,7 +173,7 @@ func TestFindAllTargets(t *testing.T) { t.Errorf("got wrong value: %v", ks) } - // Only get 1 keyspaces only for all types. + // Only get 1 keyspace for all types. ks, err = FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace2"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) if err != nil { t.Errorf("unexpected error: %v", err) From 78f1876659316440918f1be43d5454fa13671706 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Sun, 25 Feb 2024 22:39:34 +0100 Subject: [PATCH 05/10] use testify/assert Signed-off-by: Tim Vaillancourt --- go/vt/srvtopo/discover_test.go | 85 +++++++++++----------------------- 1 file changed, 27 insertions(+), 58 deletions(-) diff --git a/go/vt/srvtopo/discover_test.go b/go/vt/srvtopo/discover_test.go index f1f35e1e43d..b42ab64920e 100644 --- a/go/vt/srvtopo/discover_test.go +++ b/go/vt/srvtopo/discover_test.go @@ -18,11 +18,12 @@ package srvtopo import ( "context" - "reflect" "sort" "testing" "time" + "github.com/stretchr/testify/assert" + "vitess.io/vitess/go/stats" "vitess.io/vitess/go/vt/topo/memorytopo" @@ -65,15 +66,11 @@ func TestFindAllTargets(t *testing.T) { // No keyspace / shards. ks, err := FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if len(ks) > 0 { - t.Errorf("why did I get anything? %v", ks) - } + assert.Nil(t, err) + assert.Len(t, ks, 0) // Add one. - if err := ts.UpdateSrvKeyspace(ctx, "cell1", "test_keyspace", &topodatapb.SrvKeyspace{ + assert.Nilf(t, ts.UpdateSrvKeyspace(ctx, "cell1", "test_keyspace", &topodatapb.SrvKeyspace{ Partitions: []*topodatapb.SrvKeyspace_KeyspacePartition{ { ServedType: topodatapb.TabletType_PRIMARY, @@ -84,44 +81,34 @@ func TestFindAllTargets(t *testing.T) { }, }, }, - }); err != nil { - t.Fatalf("can't add srvKeyspace: %v", err) - } + }), "can't add srvKeyspace: %v", err) // Get it. ks, err = FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if !reflect.DeepEqual(ks, []*querypb.Target{ + assert.Nil(t, err) + assert.Equal(t, []*querypb.Target{ { Cell: "cell1", Keyspace: "test_keyspace", Shard: "test_shard0", TabletType: topodatapb.TabletType_PRIMARY, }, - }) { - t.Errorf("got wrong value: %v", ks) - } + }, ks) // Get any keyspace. ks, err = FindAllTargets(ctx, rs, "cell1", []string{}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if !reflect.DeepEqual(ks, []*querypb.Target{ + assert.Nil(t, err) + assert.Equal(t, []*querypb.Target{ { Cell: "cell1", Keyspace: "test_keyspace", Shard: "test_shard0", TabletType: topodatapb.TabletType_PRIMARY, }, - }) { - t.Errorf("got wrong value: %v", ks) - } + }, ks) // Add another one. - if err := ts.UpdateSrvKeyspace(ctx, "cell1", "test_keyspace2", &topodatapb.SrvKeyspace{ + assert.Nil(t, ts.UpdateSrvKeyspace(ctx, "cell1", "test_keyspace2", &topodatapb.SrvKeyspace{ Partitions: []*topodatapb.SrvKeyspace_KeyspacePartition{ { ServedType: topodatapb.TabletType_PRIMARY, @@ -140,17 +127,13 @@ func TestFindAllTargets(t *testing.T) { }, }, }, - }); err != nil { - t.Fatalf("can't add srvKeyspace: %v", err) - } + })) - // Get it for all types. - ks, err = FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace", "test_keyspace2"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) - if err != nil { - t.Errorf("unexpected error: %v", err) - } + // Get it for any keyspace, all types. + ks, err = FindAllTargets(ctx, rs, "cell1", []string{}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) + assert.Nil(t, err) sort.Sort(TargetArray(ks)) - if !reflect.DeepEqual(ks, []*querypb.Target{ + assert.Equal(t, []*querypb.Target{ { Cell: "cell1", Keyspace: "test_keyspace", @@ -169,16 +152,12 @@ func TestFindAllTargets(t *testing.T) { Shard: "test_shard2", TabletType: topodatapb.TabletType_REPLICA, }, - }) { - t.Errorf("got wrong value: %v", ks) - } + }, ks) // Only get 1 keyspace for all types. ks, err = FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace2"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if !reflect.DeepEqual(ks, []*querypb.Target{ + assert.Nil(t, err) + assert.Equal(t, []*querypb.Target{ { Cell: "cell1", Keyspace: "test_keyspace2", @@ -191,32 +170,22 @@ func TestFindAllTargets(t *testing.T) { Shard: "test_shard2", TabletType: topodatapb.TabletType_REPLICA, }, - }) { - t.Errorf("got wrong value: %v", ks) - } + }, ks) // Only get the REPLICA targets for any keyspace. ks, err = FindAllTargets(ctx, rs, "cell1", []string{}, []topodatapb.TabletType{topodatapb.TabletType_REPLICA}) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if !reflect.DeepEqual(ks, []*querypb.Target{ + assert.Nil(t, err) + assert.Equal(t, []*querypb.Target{ { Cell: "cell1", Keyspace: "test_keyspace2", Shard: "test_shard2", TabletType: topodatapb.TabletType_REPLICA, }, - }) { - t.Errorf("got wrong value: %v", ks) - } + }, ks) // Get non-existent keyspace. ks, err = FindAllTargets(ctx, rs, "cell1", []string{"doesnt-exist"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - if len(ks) != 0 { - t.Errorf("got non-0 length value: %v", ks) - } + assert.Nil(t, err) + assert.Len(t, ks, 0) } From f7b7f2196eb291feb575aba09d978287f601eda4 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Mon, 26 Feb 2024 01:45:33 +0100 Subject: [PATCH 06/10] optimize further Signed-off-by: Tim Vaillancourt --- go/vt/srvtopo/discover.go | 32 +++++++++----------------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/go/vt/srvtopo/discover.go b/go/vt/srvtopo/discover.go index 7c7e4c58023..a54e75f641e 100644 --- a/go/vt/srvtopo/discover.go +++ b/go/vt/srvtopo/discover.go @@ -29,37 +29,23 @@ import ( topodatapb "vitess.io/vitess/go/vt/proto/topodata" ) -// shouldFindKeyspace returns true if a keyspace should be fetched. -// true is also returned if no keyspaces are defined. -func shouldFindKeyspace(keyspaces []string, ksName string) bool { +// FindAllTargets goes through all serving shards in the topology for the provided +// keyspaces and tablet types. If keyspaces are provided all keyspaces in the topo +// are used. It returns one Target object per keyspace/shard/matching TabletType. +func FindAllTargets(ctx context.Context, ts Server, cell string, keyspaces []string, tabletTypes []topodatapb.TabletType) ([]*querypb.Target, error) { + var err error if len(keyspaces) == 0 { - return true - } - for _, keyspace := range keyspaces { - if ksName == keyspace { - return true + keyspaces, err = ts.GetSrvKeyspaceNames(ctx, cell, true) + if err != nil { + return nil, err } } - return false -} - -// FindAllTargets goes through all serving shards in the topology -// for the provided tablet types. It returns one Target object per -// keyspace / shard / matching TabletType. -func FindAllTargets(ctx context.Context, ts Server, cell string, keyspaces []string, tabletTypes []topodatapb.TabletType) ([]*querypb.Target, error) { - ksNames, err := ts.GetSrvKeyspaceNames(ctx, cell, true) - if err != nil { - return nil, err - } var targets []*querypb.Target var wg sync.WaitGroup var mu sync.Mutex var errRecorder concurrency.AllErrorRecorder - for _, ksName := range ksNames { - if !shouldFindKeyspace(keyspaces, ksName) { - continue - } + for _, ksName := range keyspaces { wg.Add(1) go func(keyspace string) { defer wg.Done() From ad6dc55fc178a9f6dfe06b2812f01375463a3466 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Mon, 26 Feb 2024 01:47:55 +0100 Subject: [PATCH 07/10] correct comment Signed-off-by: Tim Vaillancourt --- go/vt/srvtopo/discover.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/go/vt/srvtopo/discover.go b/go/vt/srvtopo/discover.go index a54e75f641e..283993e5e8e 100644 --- a/go/vt/srvtopo/discover.go +++ b/go/vt/srvtopo/discover.go @@ -29,9 +29,9 @@ import ( topodatapb "vitess.io/vitess/go/vt/proto/topodata" ) -// FindAllTargets goes through all serving shards in the topology for the provided -// keyspaces and tablet types. If keyspaces are provided all keyspaces in the topo -// are used. It returns one Target object per keyspace/shard/matching TabletType. +// FindAllTargets goes through all serving shards in the topology for the provided keyspaces +// and tablet types. If no keyspaces are provided, all available keyspaces in the topo are +// fetched. It returns one Target object per keyspace/shard/matching TabletType. func FindAllTargets(ctx context.Context, ts Server, cell string, keyspaces []string, tabletTypes []topodatapb.TabletType) ([]*querypb.Target, error) { var err error if len(keyspaces) == 0 { From 6f284ad6f370895962c0db10c0718a8aa8a5e1b4 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Mon, 26 Feb 2024 01:48:31 +0100 Subject: [PATCH 08/10] correct comment 2 Signed-off-by: Tim Vaillancourt --- go/vt/srvtopo/discover.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/srvtopo/discover.go b/go/vt/srvtopo/discover.go index 283993e5e8e..2997dc42e21 100644 --- a/go/vt/srvtopo/discover.go +++ b/go/vt/srvtopo/discover.go @@ -30,7 +30,7 @@ import ( ) // FindAllTargets goes through all serving shards in the topology for the provided keyspaces -// and tablet types. If no keyspaces are provided, all available keyspaces in the topo are +// and tablet types. If no keyspaces are provided all available keyspaces in the topo are // fetched. It returns one Target object per keyspace/shard/matching TabletType. func FindAllTargets(ctx context.Context, ts Server, cell string, keyspaces []string, tabletTypes []topodatapb.TabletType) ([]*querypb.Target, error) { var err error From 8318cea2ffaeec21c294a5da4be655b29f673080 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Tue, 27 Feb 2024 18:37:16 +0100 Subject: [PATCH 09/10] use assert.NoError for clarity Signed-off-by: Tim Vaillancourt --- go/vt/srvtopo/discover_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/go/vt/srvtopo/discover_test.go b/go/vt/srvtopo/discover_test.go index b42ab64920e..682d478742d 100644 --- a/go/vt/srvtopo/discover_test.go +++ b/go/vt/srvtopo/discover_test.go @@ -66,11 +66,11 @@ func TestFindAllTargets(t *testing.T) { // No keyspace / shards. ks, err := FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) - assert.Nil(t, err) + assert.NoError(t, err) assert.Len(t, ks, 0) // Add one. - assert.Nilf(t, ts.UpdateSrvKeyspace(ctx, "cell1", "test_keyspace", &topodatapb.SrvKeyspace{ + assert.NoError(t, ts.UpdateSrvKeyspace(ctx, "cell1", "test_keyspace", &topodatapb.SrvKeyspace{ Partitions: []*topodatapb.SrvKeyspace_KeyspacePartition{ { ServedType: topodatapb.TabletType_PRIMARY, @@ -81,11 +81,11 @@ func TestFindAllTargets(t *testing.T) { }, }, }, - }), "can't add srvKeyspace: %v", err) + })) // Get it. ks, err = FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, []*querypb.Target{ { Cell: "cell1", @@ -97,7 +97,7 @@ func TestFindAllTargets(t *testing.T) { // Get any keyspace. ks, err = FindAllTargets(ctx, rs, "cell1", []string{}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, []*querypb.Target{ { Cell: "cell1", @@ -108,7 +108,7 @@ func TestFindAllTargets(t *testing.T) { }, ks) // Add another one. - assert.Nil(t, ts.UpdateSrvKeyspace(ctx, "cell1", "test_keyspace2", &topodatapb.SrvKeyspace{ + assert.NoError(t, ts.UpdateSrvKeyspace(ctx, "cell1", "test_keyspace2", &topodatapb.SrvKeyspace{ Partitions: []*topodatapb.SrvKeyspace_KeyspacePartition{ { ServedType: topodatapb.TabletType_PRIMARY, @@ -131,7 +131,7 @@ func TestFindAllTargets(t *testing.T) { // Get it for any keyspace, all types. ks, err = FindAllTargets(ctx, rs, "cell1", []string{}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) - assert.Nil(t, err) + assert.NoError(t, err) sort.Sort(TargetArray(ks)) assert.Equal(t, []*querypb.Target{ { @@ -156,7 +156,7 @@ func TestFindAllTargets(t *testing.T) { // Only get 1 keyspace for all types. ks, err = FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace2"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, []*querypb.Target{ { Cell: "cell1", @@ -174,7 +174,7 @@ func TestFindAllTargets(t *testing.T) { // Only get the REPLICA targets for any keyspace. ks, err = FindAllTargets(ctx, rs, "cell1", []string{}, []topodatapb.TabletType{topodatapb.TabletType_REPLICA}) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, []*querypb.Target{ { Cell: "cell1", @@ -186,6 +186,6 @@ func TestFindAllTargets(t *testing.T) { // Get non-existent keyspace. ks, err = FindAllTargets(ctx, rs, "cell1", []string{"doesnt-exist"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) - assert.Nil(t, err) + assert.NoError(t, err) assert.Len(t, ks, 0) } From 612d8c4635622799256c28729628f1e3928b277e Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Tue, 27 Feb 2024 18:41:30 +0100 Subject: [PATCH 10/10] pr suggestions Signed-off-by: Tim Vaillancourt --- go/vt/srvtopo/discover_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/go/vt/srvtopo/discover_test.go b/go/vt/srvtopo/discover_test.go index 682d478742d..75c5f25cc6e 100644 --- a/go/vt/srvtopo/discover_test.go +++ b/go/vt/srvtopo/discover_test.go @@ -86,7 +86,7 @@ func TestFindAllTargets(t *testing.T) { // Get it. ks, err = FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) assert.NoError(t, err) - assert.Equal(t, []*querypb.Target{ + assert.EqualValues(t, []*querypb.Target{ { Cell: "cell1", Keyspace: "test_keyspace", @@ -96,9 +96,9 @@ func TestFindAllTargets(t *testing.T) { }, ks) // Get any keyspace. - ks, err = FindAllTargets(ctx, rs, "cell1", []string{}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) + ks, err = FindAllTargets(ctx, rs, "cell1", nil, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY}) assert.NoError(t, err) - assert.Equal(t, []*querypb.Target{ + assert.EqualValues(t, []*querypb.Target{ { Cell: "cell1", Keyspace: "test_keyspace", @@ -130,10 +130,10 @@ func TestFindAllTargets(t *testing.T) { })) // Get it for any keyspace, all types. - ks, err = FindAllTargets(ctx, rs, "cell1", []string{}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) + ks, err = FindAllTargets(ctx, rs, "cell1", nil, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) assert.NoError(t, err) sort.Sort(TargetArray(ks)) - assert.Equal(t, []*querypb.Target{ + assert.EqualValues(t, []*querypb.Target{ { Cell: "cell1", Keyspace: "test_keyspace", @@ -157,7 +157,7 @@ func TestFindAllTargets(t *testing.T) { // Only get 1 keyspace for all types. ks, err = FindAllTargets(ctx, rs, "cell1", []string{"test_keyspace2"}, []topodatapb.TabletType{topodatapb.TabletType_PRIMARY, topodatapb.TabletType_REPLICA}) assert.NoError(t, err) - assert.Equal(t, []*querypb.Target{ + assert.EqualValues(t, []*querypb.Target{ { Cell: "cell1", Keyspace: "test_keyspace2",