From eae50cb5ef94c5e8c2c5baa9bf559b10ea6cd8e1 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Mon, 17 Feb 2025 20:02:56 +0100 Subject: [PATCH 01/12] Add stats for shards watched by VTOrc Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/inst/shard_dao.go | 37 +++++++++++++ go/vt/vtorc/inst/shard_dao_test.go | 13 ++++- go/vt/vtorc/logic/keyspace_shard_discovery.go | 54 ++++++++++++++++++- 3 files changed, 102 insertions(+), 2 deletions(-) diff --git a/go/vt/vtorc/inst/shard_dao.go b/go/vt/vtorc/inst/shard_dao.go index a90eed0f509..5501ba5d7b9 100644 --- a/go/vt/vtorc/inst/shard_dao.go +++ b/go/vt/vtorc/inst/shard_dao.go @@ -29,6 +29,30 @@ import ( // ErrShardNotFound is a fixed error message used when a shard is not found in the database. var ErrShardNotFound = errors.New("shard not found") +// ReadShardNames reads the names of vitess shards for a single keyspace. +func ReadShardNames(keyspaceName string) (shardNames []string, err error) { + shardNames = make([]string, 0) + query := `select shard from vitess_shard where keyspace = ?` + args := sqlutils.Args(keyspaceName) + err = db.QueryVTOrc(query, args, func(row sqlutils.RowMap) error { + shardNames = append(shardNames, row.GetString("shard")) + return nil + }) + return shardNames, err +} + +// ReadAllShardNames reads the names of all vitess shards by keyspace. +func ReadAllShardNames() (shardNames map[string][]string, err error) { + shardNames = make(map[string][]string) + query := `select keyspace, shard from vitess_shard` + err = db.QueryVTOrc(query, nil, func(row sqlutils.RowMap) error { + ks := row.GetString("keyspace") + shardNames[ks] = append(shardNames[ks], row.GetString("shard")) + return nil + }) + return shardNames, err +} + // ReadShardPrimaryInformation reads the vitess shard record and gets the shard primary alias and timestamp. func ReadShardPrimaryInformation(keyspaceName, shardName string) (primaryAlias string, primaryTimestamp string, err error) { if err = topo.ValidateKeyspaceName(keyspaceName); err != nil { @@ -95,3 +119,16 @@ func getShardPrimaryTermStartTimeString(shard *topo.ShardInfo) string { } return protoutil.TimeFromProto(shard.PrimaryTermStartTime).UTC().String() } + +// DeleteShard deletes a shard using a keyspace and shard name. +func DeleteShard(keyspace, shard string) error { + _, err := db.ExecVTOrc(`DELETE FROM + vitess_shard + WHERE + keyspace = ? + AND shard = ?`, + keyspace, + shard, + ) + return err +} diff --git a/go/vt/vtorc/inst/shard_dao_test.go b/go/vt/vtorc/inst/shard_dao_test.go index 84f6aef7a4a..0077b3d64af 100644 --- a/go/vt/vtorc/inst/shard_dao_test.go +++ b/go/vt/vtorc/inst/shard_dao_test.go @@ -28,7 +28,7 @@ import ( "vitess.io/vitess/go/vt/vtorc/db" ) -func TestSaveAndReadShard(t *testing.T) { +func TestSaveReadAndDeleteShard(t *testing.T) { // Clear the database after the test. The easiest way to do that is to run all the initialization commands again. defer func() { db.ClearVTOrcDatabase() @@ -93,6 +93,7 @@ func TestSaveAndReadShard(t *testing.T) { require.NoError(t, err) } + // ReadShardPrimaryInformation shardPrimaryAlias, primaryTimestamp, err := ReadShardPrimaryInformation(tt.keyspaceName, tt.shardName) if tt.err != "" { require.EqualError(t, err, tt.err) @@ -101,6 +102,16 @@ func TestSaveAndReadShard(t *testing.T) { require.NoError(t, err) require.EqualValues(t, tt.primaryAliasWanted, shardPrimaryAlias) require.EqualValues(t, tt.primaryTimestampWanted, primaryTimestamp) + + // ReadShardNames + shardNames, err := ReadShardNames(tt.keyspaceName) + require.NoError(t, err) + require.Equal(t, []string{tt.shardName}, shardNames) + + // DeleteShard + require.NoError(t, DeleteShard(tt.keyspaceName, tt.shardName)) + _, _, err = ReadShardPrimaryInformation(tt.keyspaceName, tt.shardName) + require.EqualError(t, err, ErrShardNotFound.Error()) }) } } diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery.go b/go/vt/vtorc/logic/keyspace_shard_discovery.go index 8115e614418..77f3930be1e 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery.go @@ -22,14 +22,43 @@ import ( "golang.org/x/exp/maps" + "vitess.io/vitess/go/stats" "vitess.io/vitess/go/vt/log" - "vitess.io/vitess/go/vt/topo" + "vitess.io/vitess/go/vt/topo/topoproto" "vitess.io/vitess/go/vt/vtorc/inst" ) +var statsShardsWatched = stats.NewGaugesFuncWithMultiLabels("ShardsWatched", + "Keyspace/shards currently watched", + []string{"Keyspace", "Shard"}, + getShardsWatchedStats) + +// getShardsWatchedStats returns the keyspace/shards watched in a format for stats. +func getShardsWatchedStats() map[string]int64 { + shardsWatched := make(map[string]int64) + allShardNames, err := inst.ReadAllShardNames() + if err != nil { + log.Errorf("Failed to read all shard names: %+v", err) + return shardsWatched + } + for ks, shards := range allShardNames { + for _, shard := range shards { + shardsWatched[ks+"."+shard] = 1 + } + } + return shardsWatched +} + +// refreshAllKeyspacesAndShardsMu ensures RefreshAllKeyspacesAndShards +// is not executed concurrently. +var refreshAllKeyspacesAndShardsMu sync.Mutex + // RefreshAllKeyspacesAndShards reloads the keyspace and shard information for the keyspaces that vtorc is concerned with. func RefreshAllKeyspacesAndShards(ctx context.Context) error { + refreshAllKeyspacesAndShardsMu.Lock() + defer refreshAllKeyspacesAndShardsMu.Unlock() + var keyspaces []string if len(shardsToWatch) == 0 { // all known keyspaces ctx, cancel := context.WithTimeout(ctx, topo.RemoteOperationTimeout) @@ -109,6 +138,7 @@ func refreshKeyspaceHelper(ctx context.Context, keyspaceName string) error { // refreshAllShards refreshes all the shard records in the given keyspace. func refreshAllShards(ctx context.Context, keyspaceName string) error { + // get all shards for keyspace name. shardInfos, err := ts.FindAllShardsInKeyspace(ctx, keyspaceName, &topo.FindAllShardsInKeyspaceOptions{ // Fetch shard records concurrently to speed up discovery. A typical // Vitess cluster will have 1-3 vtorc instances deployed, so there is @@ -119,13 +149,35 @@ func refreshAllShards(ctx context.Context, keyspaceName string) error { log.Error(err) return err } + savedShards := make(map[string]bool, len(shardInfos)) for _, shardInfo := range shardInfos { err = inst.SaveShard(shardInfo) if err != nil { log.Error(err) return err } + savedShards[shardInfo.ShardName()] = true } + + // delete shards that were not returned by ts.FindAllShardsInKeyspace(...), + // indicating they are stale. + shards, err := inst.ReadShardNames(keyspaceName) + if err != nil { + return err + } + for _, shard := range shards { + if savedShards[shard] { + continue + } + shardName := topoproto.KeyspaceShardString(keyspaceName, shard) + log.Infof("Forgetting shard: %s", shardName) + err = inst.DeleteShard(keyspaceName, shard) + if err != nil { + log.Errorf("Failed to delete shard %s: %+v", shardName, err) + return err + } + } + return nil } From a314c0336971590d398b9e11d751fba6fa23106e Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Tue, 18 Feb 2025 00:37:50 +0100 Subject: [PATCH 02/12] add more tests Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/inst/shard_dao_test.go | 6 ++++ .../logic/keyspace_shard_discovery_test.go | 30 +++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/go/vt/vtorc/inst/shard_dao_test.go b/go/vt/vtorc/inst/shard_dao_test.go index 0077b3d64af..3e4f71efcae 100644 --- a/go/vt/vtorc/inst/shard_dao_test.go +++ b/go/vt/vtorc/inst/shard_dao_test.go @@ -108,6 +108,12 @@ func TestSaveReadAndDeleteShard(t *testing.T) { require.NoError(t, err) require.Equal(t, []string{tt.shardName}, shardNames) + // ReadAllShardNames + allShardNames, err := ReadAllShardNames() + ksShards, found := allShardNames[tt.keyspaceName] + require.True(t, found) + require.Equal(t, []string{tt.shardName}, ksShards) + // DeleteShard require.NoError(t, DeleteShard(tt.keyspaceName, tt.shardName)) _, _, err = ReadShardPrimaryInformation(tt.keyspaceName, tt.shardName) diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go index f05295416d0..bfafc200b31 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go @@ -311,3 +311,33 @@ func verifyPrimaryAlias(t *testing.T, keyspaceName, shardName string, primaryAli require.NoError(t, err) require.Equal(t, primaryAliasWanted, primaryAlias) } + +func TestRefreshAllShards(t *testing.T) { + // Store the old flags and restore on test completion + oldTs := ts + defer func() { + ts = oldTs + db.ClearVTOrcDatabase() + }() + + ctx := context.Background() + ts = memorytopo.NewServer(ctx, "zone1") + require.NoError(t, ts.CreateKeyspace(ctx, "ks1", &topodatapb.Keyspace{ + KeyspaceType: topodatapb.KeyspaceType_NORMAL, + DurabilityPolicy: policy.DurabilityNone, + })) + shards := []string{"-80", "80-"} + for _, shard := range shards { + require.NoError(t, ts.CreateShard(ctx, "ks1", shard)) + } + require.NoError(t, refreshAllShards(context.Background(), "ks1")) + shardNames, err := inst.ReadShardNames("ks1") + require.NoError(t, err) + require.Equal(t, []string{"-80", "80-"}, shardNames) + + require.NoError(t, ts.DeleteShard(ctx, "ks1", "80-")) + require.NoError(t, refreshAllShards(context.Background(), "ks1")) + shardNames, err = inst.ReadShardNames("ks1") + require.NoError(t, err) + require.Equal(t, []string{"-80"}, shardNames) +} From 730d04681987c176ee942f0499555e00811d6262 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Tue, 18 Feb 2025 00:48:29 +0100 Subject: [PATCH 03/12] cleanup Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/logic/keyspace_shard_discovery_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go index bfafc200b31..183bfc40779 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go @@ -326,17 +326,18 @@ func TestRefreshAllShards(t *testing.T) { KeyspaceType: topodatapb.KeyspaceType_NORMAL, DurabilityPolicy: policy.DurabilityNone, })) + shards := []string{"-80", "80-"} for _, shard := range shards { require.NoError(t, ts.CreateShard(ctx, "ks1", shard)) } - require.NoError(t, refreshAllShards(context.Background(), "ks1")) + require.NoError(t, refreshAllShards(ctx, "ks1")) shardNames, err := inst.ReadShardNames("ks1") require.NoError(t, err) require.Equal(t, []string{"-80", "80-"}, shardNames) require.NoError(t, ts.DeleteShard(ctx, "ks1", "80-")) - require.NoError(t, refreshAllShards(context.Background(), "ks1")) + require.NoError(t, refreshAllShards(ctx, "ks1")) shardNames, err = inst.ReadShardNames("ks1") require.NoError(t, err) require.Equal(t, []string{"-80"}, shardNames) From 474115d2092a8f335c9224445ffbc3d012cad56b Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Tue, 18 Feb 2025 20:53:40 +0100 Subject: [PATCH 04/12] fix ineffassign Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/inst/shard_dao_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/go/vt/vtorc/inst/shard_dao_test.go b/go/vt/vtorc/inst/shard_dao_test.go index 3e4f71efcae..8cf67f10ee6 100644 --- a/go/vt/vtorc/inst/shard_dao_test.go +++ b/go/vt/vtorc/inst/shard_dao_test.go @@ -110,6 +110,7 @@ func TestSaveReadAndDeleteShard(t *testing.T) { // ReadAllShardNames allShardNames, err := ReadAllShardNames() + require.NoError(t, err) ksShards, found := allShardNames[tt.keyspaceName] require.True(t, found) require.Equal(t, []string{tt.shardName}, ksShards) From 4abcdedf7b5fcc327a2fe8c5402d2e0b2fe492f6 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Mon, 24 Feb 2025 22:41:14 +0100 Subject: [PATCH 05/12] dont save shards we dont watch to backend Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/logic/keyspace_shard_discovery.go | 40 ++++++++++++++++++- .../logic/keyspace_shard_discovery_test.go | 25 ++++++++++-- 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery.go b/go/vt/vtorc/logic/keyspace_shard_discovery.go index 77f3930be1e..371cf63635d 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery.go @@ -23,6 +23,7 @@ import ( "golang.org/x/exp/maps" "vitess.io/vitess/go/stats" + "vitess.io/vitess/go/vt/key" "vitess.io/vitess/go/vt/log" "vitess.io/vitess/go/vt/topo" "vitess.io/vitess/go/vt/topo/topoproto" @@ -108,6 +109,33 @@ func RefreshKeyspaceAndShard(keyspaceName string, shardName string) error { return refreshShard(keyspaceName, shardName) } +// shouldRefreshShard returns true if a shard is within the shardsToWatch +// ranges for it's keyspace. +func shouldRefreshShard(keyspace, shardName string) (bool, error) { + if len(shardsToWatch) == 0 { + return true, nil + } + + watchRanges, found := shardsToWatch[keyspace] + if !found { + return false, nil + } + + shardRanges, err := key.ParseShardingSpec(shardName) + if err != nil { + return false, err + } + + for _, keyRange := range watchRanges { + for _, shardRange := range shardRanges { + if key.KeyRangeContainsKeyRange(keyRange, shardRange) { + return true, nil + } + } + } + return false, nil +} + // refreshKeyspace refreshes the keyspace's information for the given keyspace from the topo func refreshKeyspace(keyspaceName string) error { refreshCtx, refreshCancel := context.WithTimeout(context.Background(), topo.RemoteOperationTimeout) @@ -151,12 +179,20 @@ func refreshAllShards(ctx context.Context, keyspaceName string) error { } savedShards := make(map[string]bool, len(shardInfos)) for _, shardInfo := range shardInfos { - err = inst.SaveShard(shardInfo) + shardName := shardInfo.ShardName() + shouldRefresh, err := shouldRefreshShard(keyspaceName, shardName) if err != nil { log.Error(err) return err } - savedShards[shardInfo.ShardName()] = true + if !shouldRefresh { + continue + } + if err = inst.SaveShard(shardInfo); err != nil { + log.Error(err) + return err + } + savedShards[shardName] = true } // delete shards that were not returned by ts.FindAllShardsInKeyspace(...), diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go index 183bfc40779..f6e1f70fdae 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go @@ -56,9 +56,11 @@ func TestRefreshAllKeyspaces(t *testing.T) { // Store the old flags and restore on test completion oldTs := ts oldClustersToWatch := clustersToWatch + oldShardsToWatch := shardsToWatch defer func() { ts = oldTs clustersToWatch = oldClustersToWatch + shardsToWatch = oldShardsToWatch }() db.ClearVTOrcDatabase() @@ -314,31 +316,46 @@ func verifyPrimaryAlias(t *testing.T, keyspaceName, shardName string, primaryAli func TestRefreshAllShards(t *testing.T) { // Store the old flags and restore on test completion + oldClustersToWatch := clustersToWatch + oldShardsToWatch := shardsToWatch oldTs := ts defer func() { + clustersToWatch = oldClustersToWatch + shardsToWatch = oldShardsToWatch ts = oldTs db.ClearVTOrcDatabase() }() ctx := context.Background() ts = memorytopo.NewServer(ctx, "zone1") + require.NoError(t, initializeShardsToWatch()) require.NoError(t, ts.CreateKeyspace(ctx, "ks1", &topodatapb.Keyspace{ KeyspaceType: topodatapb.KeyspaceType_NORMAL, DurabilityPolicy: policy.DurabilityNone, })) - shards := []string{"-80", "80-"} + // test shard refresh + shards := []string{"-40", "40-80", "80-c0", "c0-"} for _, shard := range shards { require.NoError(t, ts.CreateShard(ctx, "ks1", shard)) } require.NoError(t, refreshAllShards(ctx, "ks1")) shardNames, err := inst.ReadShardNames("ks1") require.NoError(t, err) - require.Equal(t, []string{"-80", "80-"}, shardNames) + require.Equal(t, []string{"-40", "40-80", "80-c0", "c0-"}, shardNames) + + // test topo shard delete propagates + require.NoError(t, ts.DeleteShard(ctx, "ks1", "c0-")) + require.NoError(t, refreshAllShards(ctx, "ks1")) + shardNames, err = inst.ReadShardNames("ks1") + require.NoError(t, err) + require.Equal(t, []string{"-40", "40-80", "80-c0"}, shardNames) - require.NoError(t, ts.DeleteShard(ctx, "ks1", "80-")) + // test clustersToWatch filters what shards are saved + clustersToWatch = []string{"ks1/-40", "ks1/40-80"} + require.NoError(t, initializeShardsToWatch()) require.NoError(t, refreshAllShards(ctx, "ks1")) shardNames, err = inst.ReadShardNames("ks1") require.NoError(t, err) - require.Equal(t, []string{"-80"}, shardNames) + require.Equal(t, []string{"-40", "40-80"}, shardNames) } From e754104dee83b2273012aac002ed1b1efc6250ef Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Mon, 24 Feb 2025 22:48:23 +0100 Subject: [PATCH 06/12] rename/cleanup Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/logic/keyspace_shard_discovery.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery.go b/go/vt/vtorc/logic/keyspace_shard_discovery.go index 371cf63635d..fecea9f32ae 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery.go @@ -109,19 +109,19 @@ func RefreshKeyspaceAndShard(keyspaceName string, shardName string) error { return refreshShard(keyspaceName, shardName) } -// shouldRefreshShard returns true if a shard is within the shardsToWatch +// shouldWatchShard returns true if a shard is within the shardsToWatch // ranges for it's keyspace. -func shouldRefreshShard(keyspace, shardName string) (bool, error) { +func shouldWatchShard(shard *topo.ShardInfo) (bool, error) { if len(shardsToWatch) == 0 { return true, nil } - watchRanges, found := shardsToWatch[keyspace] + watchRanges, found := shardsToWatch[shard.Keyspace()] if !found { return false, nil } - shardRanges, err := key.ParseShardingSpec(shardName) + shardRanges, err := key.ParseShardingSpec(shard.ShardName()) if err != nil { return false, err } @@ -180,7 +180,7 @@ func refreshAllShards(ctx context.Context, keyspaceName string) error { savedShards := make(map[string]bool, len(shardInfos)) for _, shardInfo := range shardInfos { shardName := shardInfo.ShardName() - shouldRefresh, err := shouldRefreshShard(keyspaceName, shardName) + shouldRefresh, err := shouldWatchShard(shardInfo) if err != nil { log.Error(err) return err From 8c343b67596cc81e40f9cb2d6968112e42dd5822 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Mon, 24 Feb 2025 22:51:03 +0100 Subject: [PATCH 07/12] cleanup Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/logic/keyspace_shard_discovery.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery.go b/go/vt/vtorc/logic/keyspace_shard_discovery.go index fecea9f32ae..62e7be88c4a 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery.go @@ -179,20 +179,19 @@ func refreshAllShards(ctx context.Context, keyspaceName string) error { } savedShards := make(map[string]bool, len(shardInfos)) for _, shardInfo := range shardInfos { - shardName := shardInfo.ShardName() - shouldRefresh, err := shouldWatchShard(shardInfo) + shouldWatchShard, err := shouldWatchShard(shardInfo) if err != nil { log.Error(err) return err } - if !shouldRefresh { + if !shouldWatchShard { continue } if err = inst.SaveShard(shardInfo); err != nil { log.Error(err) return err } - savedShards[shardName] = true + savedShards[shardInfo.ShardName()] = true } // delete shards that were not returned by ts.FindAllShardsInKeyspace(...), From f4f6f05f0c41d4558c6d5ce8c9d5cf154d1b06d5 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Mon, 24 Feb 2025 22:56:39 +0100 Subject: [PATCH 08/12] simplify Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/logic/keyspace_shard_discovery.go | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery.go b/go/vt/vtorc/logic/keyspace_shard_discovery.go index 62e7be88c4a..05ef6b9530c 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery.go @@ -111,29 +111,32 @@ func RefreshKeyspaceAndShard(keyspaceName string, shardName string) error { // shouldWatchShard returns true if a shard is within the shardsToWatch // ranges for it's keyspace. -func shouldWatchShard(shard *topo.ShardInfo) (bool, error) { +func shouldWatchShard(shard *topo.ShardInfo) bool { if len(shardsToWatch) == 0 { - return true, nil + return true } watchRanges, found := shardsToWatch[shard.Keyspace()] if !found { - return false, nil + return false } shardRanges, err := key.ParseShardingSpec(shard.ShardName()) if err != nil { - return false, err + // This should never happen because we parse shard + // names when building shardsToWatch. + log.Error(err) + return false } for _, keyRange := range watchRanges { for _, shardRange := range shardRanges { if key.KeyRangeContainsKeyRange(keyRange, shardRange) { - return true, nil + return true } } } - return false, nil + return false } // refreshKeyspace refreshes the keyspace's information for the given keyspace from the topo @@ -179,12 +182,7 @@ func refreshAllShards(ctx context.Context, keyspaceName string) error { } savedShards := make(map[string]bool, len(shardInfos)) for _, shardInfo := range shardInfos { - shouldWatchShard, err := shouldWatchShard(shardInfo) - if err != nil { - log.Error(err) - return err - } - if !shouldWatchShard { + if !shouldWatchShard(shardInfo) { continue } if err = inst.SaveShard(shardInfo); err != nil { From a0a830c913b51c539665593f9d2954db60a040c8 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Mon, 24 Feb 2025 22:59:29 +0100 Subject: [PATCH 09/12] update test Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/logic/keyspace_shard_discovery_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go index f6e1f70fdae..e66f36723ec 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go @@ -352,7 +352,7 @@ func TestRefreshAllShards(t *testing.T) { require.Equal(t, []string{"-40", "40-80", "80-c0"}, shardNames) // test clustersToWatch filters what shards are saved - clustersToWatch = []string{"ks1/-40", "ks1/40-80"} + clustersToWatch = []string{"ks1/-80"} require.NoError(t, initializeShardsToWatch()) require.NoError(t, refreshAllShards(ctx, "ks1")) shardNames, err = inst.ReadShardNames("ks1") From 469cf00295d5b5d53f20dfb7d495267748850239 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Mon, 24 Feb 2025 23:10:44 +0100 Subject: [PATCH 10/12] fix comment Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/logic/keyspace_shard_discovery.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery.go b/go/vt/vtorc/logic/keyspace_shard_discovery.go index 05ef6b9530c..edb6644dfb9 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery.go @@ -123,8 +123,8 @@ func shouldWatchShard(shard *topo.ShardInfo) bool { shardRanges, err := key.ParseShardingSpec(shard.ShardName()) if err != nil { - // This should never happen because we parse shard - // names when building shardsToWatch. + // This parse should never fail because we get the shard names + // from the topo using ts.FindAllShardsInKeyspace(). log.Error(err) return false } From 8588aec730d22f679345b03e6d2869c638715c12 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Tue, 25 Feb 2025 00:43:54 +0100 Subject: [PATCH 11/12] re-use KeyRange from shard object Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/logic/keyspace_shard_discovery.go | 17 +++-------------- .../logic/keyspace_shard_discovery_test.go | 9 +++------ 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery.go b/go/vt/vtorc/logic/keyspace_shard_discovery.go index edb6644dfb9..3176f7be4a1 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery.go @@ -121,19 +121,9 @@ func shouldWatchShard(shard *topo.ShardInfo) bool { return false } - shardRanges, err := key.ParseShardingSpec(shard.ShardName()) - if err != nil { - // This parse should never fail because we get the shard names - // from the topo using ts.FindAllShardsInKeyspace(). - log.Error(err) - return false - } - for _, keyRange := range watchRanges { - for _, shardRange := range shardRanges { - if key.KeyRangeContainsKeyRange(keyRange, shardRange) { - return true - } + if key.KeyRangeContainsKeyRange(keyRange, shard.GetKeyRange()) { + return true } } return false @@ -204,8 +194,7 @@ func refreshAllShards(ctx context.Context, keyspaceName string) error { } shardName := topoproto.KeyspaceShardString(keyspaceName, shard) log.Infof("Forgetting shard: %s", shardName) - err = inst.DeleteShard(keyspaceName, shard) - if err != nil { + if err = inst.DeleteShard(keyspaceName, shard); err != nil { log.Errorf("Failed to delete shard %s: %+v", shardName, err) return err } diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go index e66f36723ec..0afd681383e 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go @@ -329,16 +329,13 @@ func TestRefreshAllShards(t *testing.T) { ctx := context.Background() ts = memorytopo.NewServer(ctx, "zone1") require.NoError(t, initializeShardsToWatch()) - require.NoError(t, ts.CreateKeyspace(ctx, "ks1", &topodatapb.Keyspace{ - KeyspaceType: topodatapb.KeyspaceType_NORMAL, - DurabilityPolicy: policy.DurabilityNone, - })) - - // test shard refresh + require.NoError(t, ts.CreateKeyspace(ctx, "ks1", keyspaceDurabilityNone)) shards := []string{"-40", "40-80", "80-c0", "c0-"} for _, shard := range shards { require.NoError(t, ts.CreateShard(ctx, "ks1", shard)) } + + // test shard refresh require.NoError(t, refreshAllShards(ctx, "ks1")) shardNames, err := inst.ReadShardNames("ks1") require.NoError(t, err) From 092ab7a366e829a8cb5ddfbac07cbbe9f2e0c9b7 Mon Sep 17 00:00:00 2001 From: Tim Vaillancourt Date: Tue, 25 Feb 2025 22:58:09 +0100 Subject: [PATCH 12/12] cleanup comments Signed-off-by: Tim Vaillancourt --- go/vt/vtorc/logic/keyspace_shard_discovery.go | 6 ++++-- go/vt/vtorc/logic/keyspace_shard_discovery_test.go | 4 ---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery.go b/go/vt/vtorc/logic/keyspace_shard_discovery.go index 3176f7be4a1..56fe551eb49 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery.go @@ -170,6 +170,8 @@ func refreshAllShards(ctx context.Context, keyspaceName string) error { log.Error(err) return err } + + // save shards that should be watched. savedShards := make(map[string]bool, len(shardInfos)) for _, shardInfo := range shardInfos { if !shouldWatchShard(shardInfo) { @@ -182,10 +184,10 @@ func refreshAllShards(ctx context.Context, keyspaceName string) error { savedShards[shardInfo.ShardName()] = true } - // delete shards that were not returned by ts.FindAllShardsInKeyspace(...), - // indicating they are stale. + // delete shards that were not saved, indicating they are stale. shards, err := inst.ReadShardNames(keyspaceName) if err != nil { + log.Error(err) return err } for _, shard := range shards { diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go index 0afd681383e..b5b09aba95e 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery_test.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery_test.go @@ -56,11 +56,9 @@ func TestRefreshAllKeyspaces(t *testing.T) { // Store the old flags and restore on test completion oldTs := ts oldClustersToWatch := clustersToWatch - oldShardsToWatch := shardsToWatch defer func() { ts = oldTs clustersToWatch = oldClustersToWatch - shardsToWatch = oldShardsToWatch }() db.ClearVTOrcDatabase() @@ -317,11 +315,9 @@ func verifyPrimaryAlias(t *testing.T, keyspaceName, shardName string, primaryAli func TestRefreshAllShards(t *testing.T) { // Store the old flags and restore on test completion oldClustersToWatch := clustersToWatch - oldShardsToWatch := shardsToWatch oldTs := ts defer func() { clustersToWatch = oldClustersToWatch - shardsToWatch = oldShardsToWatch ts = oldTs db.ClearVTOrcDatabase() }()