Fix AnyKeyspace tablet type selection#19399
Fix AnyKeyspace tablet type selection#19399Devanshusharma2005 wants to merge 5 commits intovitessio:mainfrom
AnyKeyspace tablet type selection#19399Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
|
@mhamza15 lets have a look. |
| // anyShardDestination is reused across canResolveKeyspace calls to avoid | ||
| // allocating a new slice on every invocation. | ||
| anyShardDestination = []key.ShardDestination{key.DestinationAnyShard{}} |
There was a problem hiding this comment.
If it's used only within canResolveKeyspace, then it won't cause an allocation. We should colocate it there if it's only used there.
| return true | ||
| } | ||
| _, _, err := vc.resolver.ResolveDestinations( | ||
| context.Background(), ksName, vc.tabletType, |
There was a problem hiding this comment.
We shouldn't use context.Background(). We'll need to funnel in the appropriate context, or at the very least use a short timeout.
There was a problem hiding this comment.
Can we do something like this here ? func (vc VCursorImpl) canResolveKeyspace(ksName string) bool {
if vc.resolver == nil {
return true
}
anyShardDestination := []key.ShardDestination{key.DestinationAnyShard{}}
ctx, cancel := context.WithTimeout(context.TODO(), 50time.Millisecond)
defer cancel()
_, _, err := vc.resolver.ResolveDestinations(ctx, ksName, vc.tabletType, nil, anyShardDestination)
return err == nil
} , ig we can use context.TODO when we're unclear and the surrounding is not ready for a ctx param ?
| // canResolveKeyspace checks whether the given keyspace has a SrvKeyspace partition | ||
| // for vc.tabletType. Uses ResolveDestinations which reads cached SrvKeyspace data, | ||
| // following the same code path as explicit keyspace routing (Resolver.GetKeyspaceShards). |
There was a problem hiding this comment.
| // canResolveKeyspace checks whether the given keyspace has a SrvKeyspace partition | |
| // for vc.tabletType. Uses ResolveDestinations which reads cached SrvKeyspace data, | |
| // following the same code path as explicit keyspace routing (Resolver.GetKeyspaceShards). | |
| // canResolveKeyspace checks whether the given keyspace has a SrvKeyspace partition | |
| // for vc.tabletType. |
Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
Signed-off-by: Devanshu Sharma <devanshusharma658@gmail.com>
6f726e0 to
5a4f14a
Compare
Description
This PR fixes a VTGate routing bug where "replica" queries without an explicit keyspace could fail with "no healthy tablet available".
The root cause was that AnyKeyspace() picked the first alphabetical serving keyspace without checking whether it actually had tablets for the requested type. So if the first keyspace only had PRIMARY tablets, an "replica" query would immediately fail even if another keyspace had healthy replicas.
The fix makes global routing tablet-type aware by reusing ResolveDestinations() through a small helper (canResolveKeyspace()). We filter serving keyspaces based on whether they can actually resolve for vc.tabletType, and then continue with the existing selection logic.
If filtering returns nothing, we gracefully fall back to the original behavior to preserve backwards compatibility.
Scope is intentionally small (2 files), no interface changes, and tests cover the main edge cases
Minimal change, aligns global routing with explicit routing behavior.
Related Issue(s)
Fixes : #19243
TEST RESULT
Checklist
Deployment Notes
No deployment changes required.
This is purely a VTGate routing logic fix. No changes to topology, VSchema, or tabletmanager.
AI Disclosure
I leveraged perplexity in generating tests. I Manually wrote code using grep commands to find the exact bug file and make changes. Fully understood all changes. @arthurschreiber gave enough hints in the issue.
(gofumpt command is the work of god btw ^^)