From 3485022bd25fe2c47743e886de7c0cfec743a97f Mon Sep 17 00:00:00 2001 From: Matt Layher Date: Mon, 4 Dec 2023 10:50:13 -0500 Subject: [PATCH 1/7] go/vt/topo: use 8 concurrency for FindAllShardsInKeyspace Signed-off-by: Matt Layher --- go/vt/topo/keyspace.go | 51 +++++++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/go/vt/topo/keyspace.go b/go/vt/topo/keyspace.go index feb80c374e5..43638a00e02 100755 --- a/go/vt/topo/keyspace.go +++ b/go/vt/topo/keyspace.go @@ -19,6 +19,9 @@ package topo import ( "context" "path" + "sync" + + "golang.org/x/sync/errgroup" "vitess.io/vitess/go/constants/sidecar" "vitess.io/vitess/go/vt/vterrors" @@ -278,18 +281,50 @@ func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string) return nil, vterrors.Wrapf(err, "failed to get list of shards for keyspace '%v'", keyspace) } - result := make(map[string]*ShardInfo, len(shards)) + // Keyspaces with a large number of shards and geographically distributed + // topo instances may experience significant latency fetching shard records. + // + // A prior version of this logic used unbounded concurrency to fetch shard + // records which resulted in overwhelming topo server instances: + // https://github.com/vitessio/vitess/pull/5436. + // + // However, removing the concurrency all together can cause large operations + // to fail all together due to timeout. Set a reasonable worker limit (8 as + // a starting point, chosen in December 2023) to enable concurrent record + // fetches while attempting to avoid any thundering herd problems. + var ( + mu sync.Mutex + result = make(map[string]*ShardInfo, len(shards)) + ) + + eg, ctx := errgroup.WithContext(ctx) + eg.SetLimit(8) + for _, shard := range shards { - si, err := ts.GetShard(ctx, keyspace, shard) - if err != nil { - if IsErrType(err, NoNode) { + shard := shard + + eg.Go(func() error { + si, err := ts.GetShard(ctx, keyspace, shard) + switch { + case IsErrType(err, NoNode): log.Warningf("GetShard(%v, %v) returned ErrNoNode, consider checking the topology.", keyspace, shard) - } else { - return nil, vterrors.Wrapf(err, "GetShard(%v, %v) failed", keyspace, shard) + return nil + case err == nil: + mu.Lock() + result[shard] = si + mu.Unlock() + + return nil + default: + return vterrors.Wrapf(err, "GetShard(%v, %v) failed", keyspace, shard) } - } - result[shard] = si + }) } + + if err := eg.Wait(); err != nil { + return nil, err + } + return result, nil } From e1cbed9f37d13fe100d7c39a8d75fa3124b25ccc Mon Sep 17 00:00:00 2001 From: Matt Layher Date: Mon, 4 Dec 2023 16:26:19 -0500 Subject: [PATCH 2/7] go/vt/topo: add FindAllShardsInKeyspaceConfig to tune concurrency Signed-off-by: Matt Layher --- go/vt/topo/keyspace.go | 32 +++++-- go/vt/topo/keyspace_external_test.go | 83 +++++++++++++++++++ go/vt/topo/shard.go | 9 +- go/vt/topotools/rebuild_keyspace.go | 3 +- go/vt/topotools/split.go | 5 +- go/vt/vtctl/grpcvtctldserver/server.go | 2 +- go/vt/vtctl/workflow/utils.go | 2 +- go/vt/vtorc/logic/keyspace_shard_discovery.go | 3 +- go/vt/wrangler/keyspace.go | 2 +- 9 files changed, 124 insertions(+), 17 deletions(-) create mode 100644 go/vt/topo/keyspace_external_test.go diff --git a/go/vt/topo/keyspace.go b/go/vt/topo/keyspace.go index 43638a00e02..a5abfeec2f9 100755 --- a/go/vt/topo/keyspace.go +++ b/go/vt/topo/keyspace.go @@ -273,9 +273,25 @@ func (ts *Server) UpdateKeyspace(ctx context.Context, ki *KeyspaceInfo) error { return nil } -// FindAllShardsInKeyspace reads and returns all the existing shards in -// a keyspace. It doesn't take any lock. -func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string) (map[string]*ShardInfo, error) { +// FindAllShardsInKeyspaceConfig controls the behavior of +// Server.FindAllShardsInKeyspace. +type FindAllShardsInKeyspaceConfig struct { + // Concurrency controls the maximum number of concurrent calls to GetShard. + // If unspecified, Concurrency is set to 1. + Concurrency int +} + +// FindAllShardsInKeyspace reads and returns all the existing shards in a +// keyspace. It doesn't take any lock. +// +// If cfg is non-nil, it is used to configure the method's behavior. Otherwise, +// a default configuration is used. +func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string, cfg *FindAllShardsInKeyspaceConfig) (map[string]*ShardInfo, error) { + if cfg == nil || cfg.Concurrency == 0 { + // Apply defaults. + cfg = &FindAllShardsInKeyspaceConfig{Concurrency: 1} + } + shards, err := ts.GetShardNames(ctx, keyspace) if err != nil { return nil, vterrors.Wrapf(err, "failed to get list of shards for keyspace '%v'", keyspace) @@ -289,16 +305,16 @@ func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string) // https://github.com/vitessio/vitess/pull/5436. // // However, removing the concurrency all together can cause large operations - // to fail all together due to timeout. Set a reasonable worker limit (8 as - // a starting point, chosen in December 2023) to enable concurrent record - // fetches while attempting to avoid any thundering herd problems. + // to fail all together due to timeout. The caller chooses the appropriate + // concurrency level so that certain paths can be optimized (such as vtctld + // RebuildKeyspace calls, which do not run on every vttablet). var ( mu sync.Mutex result = make(map[string]*ShardInfo, len(shards)) ) eg, ctx := errgroup.WithContext(ctx) - eg.SetLimit(8) + eg.SetLimit(cfg.Concurrency) for _, shard := range shards { shard := shard @@ -354,7 +370,7 @@ func (ts *Server) GetServingShards(ctx context.Context, keyspace string) ([]*Sha // GetOnlyShard returns the single ShardInfo of an unsharded keyspace. func (ts *Server) GetOnlyShard(ctx context.Context, keyspace string) (*ShardInfo, error) { - allShards, err := ts.FindAllShardsInKeyspace(ctx, keyspace) + allShards, err := ts.FindAllShardsInKeyspace(ctx, keyspace, nil) if err != nil { return nil, err } diff --git a/go/vt/topo/keyspace_external_test.go b/go/vt/topo/keyspace_external_test.go new file mode 100644 index 00000000000..4c3f4fd0472 --- /dev/null +++ b/go/vt/topo/keyspace_external_test.go @@ -0,0 +1,83 @@ +/* +Copyright 2023 The Vitess Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package topo_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "vitess.io/vitess/go/vt/key" + topodatapb "vitess.io/vitess/go/vt/proto/topodata" + "vitess.io/vitess/go/vt/topo" + "vitess.io/vitess/go/vt/topo/memorytopo" +) + +func TestServerFindAllShardsInKeyspace(t *testing.T) { + tests := []struct { + name string + shards int + cfg *topo.FindAllShardsInKeyspaceConfig + }{ + { + name: "unsharded", + shards: 1, + // Make sure the defaults apply as expected. + cfg: nil, + }, + { + name: "sharded", + shards: 32, + cfg: &topo.FindAllShardsInKeyspaceConfig{Concurrency: 8}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ts := memorytopo.NewServer(ctx) + defer ts.Close() + + // Create an ephemeral keyspace and generate shard records within + // the keyspace to fetch later. + const keyspace = "keyspace" + require.NoError(t, ts.CreateKeyspace(ctx, keyspace, &topodatapb.Keyspace{})) + + shards, err := key.GenerateShardRanges(tt.shards) + require.NoError(t, err) + + for _, s := range shards { + require.NoError(t, ts.CreateShard(ctx, keyspace, s)) + } + + // Verify that we return a complete list of shards and that each + // key range is present in the output. + out, err := ts.FindAllShardsInKeyspace(ctx, keyspace, tt.cfg) + require.NoError(t, err) + require.Len(t, out, tt.shards) + + for _, s := range shards { + if _, ok := out[s]; !ok { + t.Errorf("shard %q was not found", s) + } + } + }) + } +} diff --git a/go/vt/topo/shard.go b/go/vt/topo/shard.go index 183ed409bbb..b1fd09c9b69 100644 --- a/go/vt/topo/shard.go +++ b/go/vt/topo/shard.go @@ -314,7 +314,14 @@ func (ts *Server) CreateShard(ctx context.Context, keyspace, shard string) (err // Set primary as serving only if its keyrange doesn't overlap // with other shards. This applies to unsharded keyspaces also value.IsPrimaryServing = true - sis, err := ts.FindAllShardsInKeyspace(ctx, keyspace) + sis, err := ts.FindAllShardsInKeyspace(ctx, keyspace, &FindAllShardsInKeyspaceConfig{ + // Assume that CreateShard may be called by many vttablets concurrently + // in a large, sharded keyspace. Do not apply concurrency to avoid + // overwhelming the toposerver. + // + // See: https://github.com/vitessio/vitess/pull/5436. + Concurrency: 1, + }) if err != nil && !IsErrType(err, NoNode) { return err } diff --git a/go/vt/topotools/rebuild_keyspace.go b/go/vt/topotools/rebuild_keyspace.go index d58ce0b7160..fdda3a4f077 100644 --- a/go/vt/topotools/rebuild_keyspace.go +++ b/go/vt/topotools/rebuild_keyspace.go @@ -64,7 +64,8 @@ func RebuildKeyspaceLocked(ctx context.Context, log logutil.Logger, ts *topo.Ser } } - shards, err := ts.FindAllShardsInKeyspace(ctx, keyspace) + // TODO(mdlayher): apply concurrency. + shards, err := ts.FindAllShardsInKeyspace(ctx, keyspace, nil) if err != nil { return err } diff --git a/go/vt/topotools/split.go b/go/vt/topotools/split.go index ace3dda94a7..0671c2c5cb8 100644 --- a/go/vt/topotools/split.go +++ b/go/vt/topotools/split.go @@ -17,12 +17,11 @@ limitations under the License. package topotools import ( + "context" "errors" "fmt" "sort" - "context" - "vitess.io/vitess/go/vt/key" topodatapb "vitess.io/vitess/go/vt/proto/topodata" "vitess.io/vitess/go/vt/topo" @@ -119,7 +118,7 @@ func OverlappingShardsForShard(os []*OverlappingShards, shardName string) *Overl // will return an error). // If shards don't perfectly overlap, they are not returned. func FindOverlappingShards(ctx context.Context, ts *topo.Server, keyspace string) ([]*OverlappingShards, error) { - shardMap, err := ts.FindAllShardsInKeyspace(ctx, keyspace) + shardMap, err := ts.FindAllShardsInKeyspace(ctx, keyspace, nil) if err != nil { return nil, err } diff --git a/go/vt/vtctl/grpcvtctldserver/server.go b/go/vt/vtctl/grpcvtctldserver/server.go index caa99d5e8c8..df7185417e9 100644 --- a/go/vt/vtctl/grpcvtctldserver/server.go +++ b/go/vt/vtctl/grpcvtctldserver/server.go @@ -1243,7 +1243,7 @@ func (s *VtctldServer) FindAllShardsInKeyspace(ctx context.Context, req *vtctlda span.Annotate("keyspace", req.Keyspace) - result, err := s.ts.FindAllShardsInKeyspace(ctx, req.Keyspace) + result, err := s.ts.FindAllShardsInKeyspace(ctx, req.Keyspace, nil) if err != nil { return nil, err } diff --git a/go/vt/vtctl/workflow/utils.go b/go/vt/vtctl/workflow/utils.go index 1a723c6192c..4d1a3c5df4d 100644 --- a/go/vt/vtctl/workflow/utils.go +++ b/go/vt/vtctl/workflow/utils.go @@ -86,7 +86,7 @@ func getTablesInKeyspace(ctx context.Context, ts *topo.Server, tmc tmclient.Tabl // validateNewWorkflow ensures that the specified workflow doesn't already exist // in the keyspace. func validateNewWorkflow(ctx context.Context, ts *topo.Server, tmc tmclient.TabletManagerClient, keyspace, workflow string) error { - allshards, err := ts.FindAllShardsInKeyspace(ctx, keyspace) + allshards, err := ts.FindAllShardsInKeyspace(ctx, keyspace, nil) if err != nil { return err } diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery.go b/go/vt/vtorc/logic/keyspace_shard_discovery.go index c79ace5bdc3..48d8944d517 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery.go @@ -124,7 +124,8 @@ 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 { - shardInfos, err := ts.FindAllShardsInKeyspace(ctx, keyspaceName) + // TODO(mdlayher): apply concurrency. + shardInfos, err := ts.FindAllShardsInKeyspace(ctx, keyspaceName, nil) if err != nil { log.Error(err) return err diff --git a/go/vt/wrangler/keyspace.go b/go/vt/wrangler/keyspace.go index 7f3f00da4f8..a5f7d6ae0bf 100644 --- a/go/vt/wrangler/keyspace.go +++ b/go/vt/wrangler/keyspace.go @@ -44,7 +44,7 @@ const ( // validateNewWorkflow ensures that the specified workflow doesn't already exist // in the keyspace. func (wr *Wrangler) validateNewWorkflow(ctx context.Context, keyspace, workflow string) error { - allshards, err := wr.ts.FindAllShardsInKeyspace(ctx, keyspace) + allshards, err := wr.ts.FindAllShardsInKeyspace(ctx, keyspace, nil) if err != nil { return err } From 4a0a5bd9be9367b7ba4858c5c0f7d7fb46e06482 Mon Sep 17 00:00:00 2001 From: Matt Layher Date: Mon, 4 Dec 2023 17:08:50 -0500 Subject: [PATCH 3/7] go/vt: use 8 concurrency for RebuildKeyspace and vtorc discovery Signed-off-by: Matt Layher --- go/vt/topo/keyspace.go | 9 ++++++--- go/vt/topotools/rebuild_keyspace.go | 16 +++++++++++----- go/vt/vtorc/logic/keyspace_shard_discovery.go | 8 ++++++-- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/go/vt/topo/keyspace.go b/go/vt/topo/keyspace.go index a5abfeec2f9..1388a2c7562 100755 --- a/go/vt/topo/keyspace.go +++ b/go/vt/topo/keyspace.go @@ -287,9 +287,12 @@ type FindAllShardsInKeyspaceConfig struct { // If cfg is non-nil, it is used to configure the method's behavior. Otherwise, // a default configuration is used. func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string, cfg *FindAllShardsInKeyspaceConfig) (map[string]*ShardInfo, error) { - if cfg == nil || cfg.Concurrency == 0 { - // Apply defaults. - cfg = &FindAllShardsInKeyspaceConfig{Concurrency: 1} + // Apply any necessary defaults. + if cfg == nil { + cfg = &FindAllShardsInKeyspaceConfig{} + } + if cfg.Concurrency == 0 { + cfg.Concurrency = 1 } shards, err := ts.GetShardNames(ctx, keyspace) diff --git a/go/vt/topotools/rebuild_keyspace.go b/go/vt/topotools/rebuild_keyspace.go index fdda3a4f077..32a26a4db18 100644 --- a/go/vt/topotools/rebuild_keyspace.go +++ b/go/vt/topotools/rebuild_keyspace.go @@ -30,14 +30,16 @@ import ( ) // RebuildKeyspace rebuilds the serving graph data while locking out other changes. -func RebuildKeyspace(ctx context.Context, log logutil.Logger, ts *topo.Server, keyspace string, cells []string, allowPartial bool) (err error) { +func RebuildKeyspace(ctx context.Context, _ logutil.Logger, ts *topo.Server, keyspace string, cells []string, allowPartial bool) (err error) { + // TODO: logutil.Logger is unused, clean up call sites. + ctx, unlock, lockErr := ts.LockKeyspace(ctx, keyspace, "RebuildKeyspace") if lockErr != nil { return lockErr } defer unlock(&err) - return RebuildKeyspaceLocked(ctx, log, ts, keyspace, cells, allowPartial) + return RebuildKeyspaceLocked(ctx, ts, keyspace, cells, allowPartial) } // RebuildKeyspaceLocked should only be used with an action lock on the keyspace @@ -46,7 +48,7 @@ func RebuildKeyspace(ctx context.Context, log logutil.Logger, ts *topo.Server, k // // Take data from the global keyspace and rebuild the local serving // copies in each cell. -func RebuildKeyspaceLocked(ctx context.Context, log logutil.Logger, ts *topo.Server, keyspace string, cells []string, allowPartial bool) error { +func RebuildKeyspaceLocked(ctx context.Context, ts *topo.Server, keyspace string, cells []string, allowPartial bool) error { if err := topo.CheckKeyspaceLocked(ctx, keyspace); err != nil { return err } @@ -64,8 +66,12 @@ func RebuildKeyspaceLocked(ctx context.Context, log logutil.Logger, ts *topo.Ser } } - // TODO(mdlayher): apply concurrency. - shards, err := ts.FindAllShardsInKeyspace(ctx, keyspace, nil) + shards, err := ts.FindAllShardsInKeyspace(ctx, keyspace, &topo.FindAllShardsInKeyspaceConfig{ + // Fetch shard records concurrently to speed up the rebuild process. + // This call is invoked by the first tablet in a given keyspace or + // manually via vtctld, so there is little risk of a thundering herd. + Concurrency: 8, + }) if err != nil { return err } diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery.go b/go/vt/vtorc/logic/keyspace_shard_discovery.go index 48d8944d517..9d6ed0d8e8c 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery.go @@ -124,8 +124,12 @@ 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 { - // TODO(mdlayher): apply concurrency. - shardInfos, err := ts.FindAllShardsInKeyspace(ctx, keyspaceName, nil) + shardInfos, err := ts.FindAllShardsInKeyspace(ctx, keyspaceName, &topo.FindAllShardsInKeyspaceConfig{ + // Fetch shard records concurrently to speed up discovery. A typical + // Vitess cluster will have 1-3 vtorc instances deployed, so there is + // little risk of a thundering herd. + Concurrency: 8, + }) if err != nil { log.Error(err) return err From 2640368bf2c8ede2f704f10e25f8770020ae3465 Mon Sep 17 00:00:00 2001 From: Matt Layher Date: Tue, 5 Dec 2023 10:22:12 -0500 Subject: [PATCH 4/7] go/vt/topo: code review feedback Signed-off-by: Matt Layher --- go/vt/topo/keyspace.go | 18 +++++++++--------- go/vt/topo/keyspace_external_test.go | 10 ++++++++-- go/vt/topo/shard.go | 2 +- go/vt/topotools/rebuild_keyspace.go | 2 +- go/vt/vtorc/logic/keyspace_shard_discovery.go | 2 +- 5 files changed, 20 insertions(+), 14 deletions(-) diff --git a/go/vt/topo/keyspace.go b/go/vt/topo/keyspace.go index 1388a2c7562..cc309a97040 100755 --- a/go/vt/topo/keyspace.go +++ b/go/vt/topo/keyspace.go @@ -273,11 +273,11 @@ func (ts *Server) UpdateKeyspace(ctx context.Context, ki *KeyspaceInfo) error { return nil } -// FindAllShardsInKeyspaceConfig controls the behavior of +// FindAllShardsInKeyspaceOptions controls the behavior of // Server.FindAllShardsInKeyspace. -type FindAllShardsInKeyspaceConfig struct { +type FindAllShardsInKeyspaceOptions struct { // Concurrency controls the maximum number of concurrent calls to GetShard. - // If unspecified, Concurrency is set to 1. + // If <= 0, Concurrency is set to 1. Concurrency int } @@ -286,13 +286,13 @@ type FindAllShardsInKeyspaceConfig struct { // // If cfg is non-nil, it is used to configure the method's behavior. Otherwise, // a default configuration is used. -func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string, cfg *FindAllShardsInKeyspaceConfig) (map[string]*ShardInfo, error) { +func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string, opt *FindAllShardsInKeyspaceOptions) (map[string]*ShardInfo, error) { // Apply any necessary defaults. - if cfg == nil { - cfg = &FindAllShardsInKeyspaceConfig{} + if opt == nil { + opt = &FindAllShardsInKeyspaceOptions{} } - if cfg.Concurrency == 0 { - cfg.Concurrency = 1 + if opt.Concurrency <= 0 { + opt.Concurrency = 1 } shards, err := ts.GetShardNames(ctx, keyspace) @@ -317,7 +317,7 @@ func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string, ) eg, ctx := errgroup.WithContext(ctx) - eg.SetLimit(cfg.Concurrency) + eg.SetLimit(opt.Concurrency) for _, shard := range shards { shard := shard diff --git a/go/vt/topo/keyspace_external_test.go b/go/vt/topo/keyspace_external_test.go index 4c3f4fd0472..86db7bf833b 100644 --- a/go/vt/topo/keyspace_external_test.go +++ b/go/vt/topo/keyspace_external_test.go @@ -32,8 +32,14 @@ func TestServerFindAllShardsInKeyspace(t *testing.T) { tests := []struct { name string shards int - cfg *topo.FindAllShardsInKeyspaceConfig + cfg *topo.FindAllShardsInKeyspaceOptions }{ + { + name: "negative concurrency", + shards: 1, + // Ensure this doesn't panic. + cfg: &topo.FindAllShardsInKeyspaceOptions{Concurrency: -1}, + }, { name: "unsharded", shards: 1, @@ -43,7 +49,7 @@ func TestServerFindAllShardsInKeyspace(t *testing.T) { { name: "sharded", shards: 32, - cfg: &topo.FindAllShardsInKeyspaceConfig{Concurrency: 8}, + cfg: &topo.FindAllShardsInKeyspaceOptions{Concurrency: 8}, }, } diff --git a/go/vt/topo/shard.go b/go/vt/topo/shard.go index b1fd09c9b69..752001438f4 100644 --- a/go/vt/topo/shard.go +++ b/go/vt/topo/shard.go @@ -314,7 +314,7 @@ func (ts *Server) CreateShard(ctx context.Context, keyspace, shard string) (err // Set primary as serving only if its keyrange doesn't overlap // with other shards. This applies to unsharded keyspaces also value.IsPrimaryServing = true - sis, err := ts.FindAllShardsInKeyspace(ctx, keyspace, &FindAllShardsInKeyspaceConfig{ + sis, err := ts.FindAllShardsInKeyspace(ctx, keyspace, &FindAllShardsInKeyspaceOptions{ // Assume that CreateShard may be called by many vttablets concurrently // in a large, sharded keyspace. Do not apply concurrency to avoid // overwhelming the toposerver. diff --git a/go/vt/topotools/rebuild_keyspace.go b/go/vt/topotools/rebuild_keyspace.go index 32a26a4db18..d9a4440e1a8 100644 --- a/go/vt/topotools/rebuild_keyspace.go +++ b/go/vt/topotools/rebuild_keyspace.go @@ -66,7 +66,7 @@ func RebuildKeyspaceLocked(ctx context.Context, ts *topo.Server, keyspace string } } - shards, err := ts.FindAllShardsInKeyspace(ctx, keyspace, &topo.FindAllShardsInKeyspaceConfig{ + shards, err := ts.FindAllShardsInKeyspace(ctx, keyspace, &topo.FindAllShardsInKeyspaceOptions{ // Fetch shard records concurrently to speed up the rebuild process. // This call is invoked by the first tablet in a given keyspace or // manually via vtctld, so there is little risk of a thundering herd. diff --git a/go/vt/vtorc/logic/keyspace_shard_discovery.go b/go/vt/vtorc/logic/keyspace_shard_discovery.go index 9d6ed0d8e8c..b1e93fe2a01 100644 --- a/go/vt/vtorc/logic/keyspace_shard_discovery.go +++ b/go/vt/vtorc/logic/keyspace_shard_discovery.go @@ -124,7 +124,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 { - shardInfos, err := ts.FindAllShardsInKeyspace(ctx, keyspaceName, &topo.FindAllShardsInKeyspaceConfig{ + 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 // little risk of a thundering herd. From b8184a76ae93f2582f5e4248da69f82f18cf21a5 Mon Sep 17 00:00:00 2001 From: Matt Layher Date: Tue, 5 Dec 2023 10:23:27 -0500 Subject: [PATCH 5/7] go/vt/topo: cfg -> opt Signed-off-by: Matt Layher --- go/vt/topo/keyspace.go | 4 ++-- go/vt/topo/keyspace_external_test.go | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/go/vt/topo/keyspace.go b/go/vt/topo/keyspace.go index cc309a97040..e062eb4ce3f 100755 --- a/go/vt/topo/keyspace.go +++ b/go/vt/topo/keyspace.go @@ -284,8 +284,8 @@ type FindAllShardsInKeyspaceOptions struct { // FindAllShardsInKeyspace reads and returns all the existing shards in a // keyspace. It doesn't take any lock. // -// If cfg is non-nil, it is used to configure the method's behavior. Otherwise, -// a default configuration is used. +// If opt is non-nil, it is used to configure the method's behavior. Otherwise, +// the default options are used. func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string, opt *FindAllShardsInKeyspaceOptions) (map[string]*ShardInfo, error) { // Apply any necessary defaults. if opt == nil { diff --git a/go/vt/topo/keyspace_external_test.go b/go/vt/topo/keyspace_external_test.go index 86db7bf833b..064c4cba93b 100644 --- a/go/vt/topo/keyspace_external_test.go +++ b/go/vt/topo/keyspace_external_test.go @@ -32,24 +32,24 @@ func TestServerFindAllShardsInKeyspace(t *testing.T) { tests := []struct { name string shards int - cfg *topo.FindAllShardsInKeyspaceOptions + opt *topo.FindAllShardsInKeyspaceOptions }{ { name: "negative concurrency", shards: 1, // Ensure this doesn't panic. - cfg: &topo.FindAllShardsInKeyspaceOptions{Concurrency: -1}, + opt: &topo.FindAllShardsInKeyspaceOptions{Concurrency: -1}, }, { name: "unsharded", shards: 1, // Make sure the defaults apply as expected. - cfg: nil, + opt: nil, }, { name: "sharded", shards: 32, - cfg: &topo.FindAllShardsInKeyspaceOptions{Concurrency: 8}, + opt: &topo.FindAllShardsInKeyspaceOptions{Concurrency: 8}, }, } @@ -75,7 +75,7 @@ func TestServerFindAllShardsInKeyspace(t *testing.T) { // Verify that we return a complete list of shards and that each // key range is present in the output. - out, err := ts.FindAllShardsInKeyspace(ctx, keyspace, tt.cfg) + out, err := ts.FindAllShardsInKeyspace(ctx, keyspace, tt.opt) require.NoError(t, err) require.Len(t, out, tt.shards) From db2798334810452b94eb275bec1eb7386cf0a0e9 Mon Sep 17 00:00:00 2001 From: Matt Layher Date: Tue, 5 Dec 2023 10:24:27 -0500 Subject: [PATCH 6/7] go/vt/topo: all together -> altogether Signed-off-by: Matt Layher --- go/vt/topo/keyspace.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/go/vt/topo/keyspace.go b/go/vt/topo/keyspace.go index e062eb4ce3f..2bdd616a261 100755 --- a/go/vt/topo/keyspace.go +++ b/go/vt/topo/keyspace.go @@ -307,9 +307,9 @@ func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string, // records which resulted in overwhelming topo server instances: // https://github.com/vitessio/vitess/pull/5436. // - // However, removing the concurrency all together can cause large operations - // to fail all together due to timeout. The caller chooses the appropriate - // concurrency level so that certain paths can be optimized (such as vtctld + // However, removing the concurrency altogether can cause large operations + // to fail due to timeout. The caller chooses the appropriate concurrency + // level so that certain paths can be optimized (such as vtctld // RebuildKeyspace calls, which do not run on every vttablet). var ( mu sync.Mutex From 3ba755139bcf2f3e5cc99cec8644b651ce102eec Mon Sep 17 00:00:00 2001 From: Matt Layher Date: Tue, 5 Dec 2023 10:26:22 -0500 Subject: [PATCH 7/7] go/vt/topotools: restore logutil.logger Signed-off-by: Matt Layher --- go/vt/topotools/rebuild_keyspace.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/go/vt/topotools/rebuild_keyspace.go b/go/vt/topotools/rebuild_keyspace.go index d9a4440e1a8..09df8b8fadc 100644 --- a/go/vt/topotools/rebuild_keyspace.go +++ b/go/vt/topotools/rebuild_keyspace.go @@ -30,16 +30,14 @@ import ( ) // RebuildKeyspace rebuilds the serving graph data while locking out other changes. -func RebuildKeyspace(ctx context.Context, _ logutil.Logger, ts *topo.Server, keyspace string, cells []string, allowPartial bool) (err error) { - // TODO: logutil.Logger is unused, clean up call sites. - +func RebuildKeyspace(ctx context.Context, log logutil.Logger, ts *topo.Server, keyspace string, cells []string, allowPartial bool) (err error) { ctx, unlock, lockErr := ts.LockKeyspace(ctx, keyspace, "RebuildKeyspace") if lockErr != nil { return lockErr } defer unlock(&err) - return RebuildKeyspaceLocked(ctx, ts, keyspace, cells, allowPartial) + return RebuildKeyspaceLocked(ctx, log, ts, keyspace, cells, allowPartial) } // RebuildKeyspaceLocked should only be used with an action lock on the keyspace @@ -48,7 +46,7 @@ func RebuildKeyspace(ctx context.Context, _ logutil.Logger, ts *topo.Server, key // // Take data from the global keyspace and rebuild the local serving // copies in each cell. -func RebuildKeyspaceLocked(ctx context.Context, ts *topo.Server, keyspace string, cells []string, allowPartial bool) error { +func RebuildKeyspaceLocked(ctx context.Context, log logutil.Logger, ts *topo.Server, keyspace string, cells []string, allowPartial bool) error { if err := topo.CheckKeyspaceLocked(ctx, keyspace); err != nil { return err }