-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Filter by keyspace earlier in tabletgateway
s WaitForTablets(...)
#15347
Filter by keyspace earlier in tabletgateway
s WaitForTablets(...)
#15347
Conversation
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15347 +/- ##
==========================================
+ Coverage 65.39% 65.43% +0.03%
==========================================
Files 1561 1561
Lines 193524 193630 +106
==========================================
+ Hits 126563 126696 +133
+ Misses 66961 66934 -27 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I only had a few very minor comments/nits. Let me know what you think and I can come back to this quickly and help to get it merged.
Thanks, @timvaillancourt !
@timvaillancourt do you mind creating a real quick issue as well for this? 🙏 |
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @timvaillancourt ! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I'm loving these little optimizations that y'all have been doing.
…itessio#15347) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
…itessio#15347) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
…itessio#15347) (#229) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
…izations (#227) * Load `--grpc_auth_static_client_creds` file once (vitessio#15030) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Filter by keyspace earlier in `tabletgateway`s `WaitForTablets(...)` (vitessio#15347) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Limit concurrent creation of healthcheck gRPC connections (vitessio#15053) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * go mod tidy Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Update MySQL apt package and GPG signature (vitessio#14785) Signed-off-by: Matt Lord <mattalord@gmail.com> * remove unrelated workflow files from v20 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Signed-off-by: Matt Lord <mattalord@gmail.com> Co-authored-by: Matt Lord <mattalord@gmail.com>
…itessio#15347) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Description
This PR causes the
tabletgateway
.WaitForTablets(...)
method to filter targets by keyspace (set by--keyspaces_to_watch
) earlier in the processCurrently, the logic under
.WaitForTablets(...)
:srvtopo.FindAllTargets(...)
to fetch the targets by-cell for any keyspace (even if--keyspaces_to_watch
is set).GetSrvKeyspace(...)
on the toposerver for every keyspace in goroutines w/wait group--keyspaces_to_watch
are removed fromtargets []*querypb.Target
Instead this PR implements the filtering in
srvtopo.FindAllTargets(...)
by not-fetching keyspaces from the topo that will just get filtered anyways. This can avoid topo calls and unnecessary filtering of results on deployments with many keyspaces and a subset of keyspaces set as--keyspaces_to_watch
The changed codepath and signatures are only called by
vtgate
/tabletgateway
so this change had a limited impactRelated Issue(s)
tabletgateway
'sWaitForTablets(...)
#15373Checklist