go/vt/topo: enable concurrency for FindAllShardsInKeyspace

[mdlayher]

Description

Speed up FindAllShardsInKeyspace by employing caller-controlled concurrency to avoid 30+ second runtimes for this call.

I've added a new struct for configuration here since certain cases such as CreateShard should not make concurrent calls in order to avoid a thundering herd on the topo.

Related Issue(s)

Fixes #14669.

Introduces some limited concurrency rather than the unbounded concurrency which was removed in #5436.

Checklist

• "Backport to:" labels have been added if this change should be back-ported
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on the CI
• Documentation was added or is not required

Deployment Notes

n/a

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[vmg]

Looks good!

[deepthi]

In the 256 shard case, we might end up with 256*8 concurrent requests to the topo. Is that expected to be safe?

[mdlayher]

Edit: my read was was incorrect, see Deepthi's reply below.

---

I don't believe that's true based on my read here:

func (ts *Server) FindAllShardsInKeyspace(ctx context.Context, keyspace string) (map[string]*ShardInfo, error) {
	shards, err := ts.GetShardNames(ctx, keyspace)

This accepts a single keyspace name as input and fetches its component shards.

	eg, ctx := errgroup.WithContext(ctx)
	eg.SetLimit(8)

	for _, shard := range shards {
		shard := shard

		eg.Go(func() error {

The loop here will begin iterating over those shards while 8 goroutines execute concurrently to fetch the shard records.

It's the same number of total calls as before, but by scheduling 8 calls at a time rather than 1, we should see a significant speedup.

---

edit: do you mean that each tablet invokes this code as well?

[deepthi]

do you mean that each tablet invokes this code as well?

That is what the original issue was running into. Tablet initialization -> CreateShard -> FindAllShardsInKeyspace
So in fact it could be 256 * 8 * tablets per shard.

[mdlayher]

One option would be to expose the concurrency as a parameter to the call.

The CreateShard uses can use a concurrency of 1 to avoid an already thundering herd while vtctld uses can use 8 instead.

[deepthi]

One option would be to expose the concurrency as a parameter to the call.

The CreateShard uses can use a concurrency of 1 to avoid an already thundering herd while vtctld uses can use 8 instead.

I can't say that I like this idea, but on the other hand, I don't think there's a better way than this though :(

[mdlayher]

I have retitled the PR and updated the description to note that the concurrency is now tunable on a per-call-site basis.

The two places I can think of that really benefit from this based on what we've seen are:

• vtctld RebuildKeyspaceGraph
• vtorc discovery

Any objections to setting a concurrency of 8 for both of those as a starting point? See the TODOs.

[mdlayher]

Ready to go!

[ajm188]

fix the potential panic and we're good to go! everything else is nitpicks, take it or leave it :)

[ajm188]

extreme nit: can technically skip one extra statement by setting the default when cfg is nil

Suggested change

	if cfg == nil {
	cfg = &FindAllShardsInKeyspaceConfig{}
	}
	if cfg.Concurrency == 0 {
	cfg.Concurrency = 1
	}
	if cfg == nil {
	cfg = &FindAllShardsInKeyspaceConfig{Concurrency: 1}
	}
	if cfg.Concurrency == 0 {
	cfg.Concurrency = 1
	}

[mdlayher]

Ack, the one reason I didn't do this is because you'd have to duplicate any future options in the same manner. I'll leave this as is.