-
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
vtgate
: support filtering tablets by tablet-tags
#15911
base: main
Are you sure you want to change the base?
vtgate
: support filtering tablets by tablet-tags
#15911
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
|
vtgate
: support filtering tablets by tablet-tags
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15911 +/- ##
==========================================
- Coverage 68.40% 68.24% -0.17%
==========================================
Files 1556 1541 -15
Lines 195121 197188 +2067
==========================================
+ Hits 133479 134562 +1083
- Misses 61642 62626 +984 ☔ 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>
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>
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.
In general I tend to be thrifty about adding new flags, but this seems like a logical extension of the tags feature of the tablet (which already exists for whatever reason), so I'm not objecting to this.
The implementation and unit tests look fine. I'll leave it to you to investigate and fix the unit tests.
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.
Left a comment on something i am not sure about. Otherwise, besides the failing tests LGTM.
We should probably do a single website PR that gathers the new flags introduced by this PR and #15919.
} | ||
topoWatchers = append(topoWatchers, NewTopologyWatcher(ctx, topoServer, hc, filter, c, refreshInterval, refreshKnownTablets, topo.DefaultConcurrency)) | ||
topoWatchers = append(topoWatchers, NewTopologyWatcher(ctx, topoServer, hc, filters, c, refreshInterval, refreshKnownTablets, topo.DefaultConcurrency)) |
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.
Shouldn't we reset the filters
slice here? Otherwise we keep appending the same filters over and over again for each cells.
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.
@frouioui good catch! yes, this should be a reset each iteration of the loop, updated 👍
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.
@frouioui re-requested review on the fix 🙇
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Description
This PR implements the RFC: #15908 by:
vtgate
flag:--tablet-filter-tags StringMap
for a list of tablet-tag KVs to matchThis support works in addition (a secondary filter) to the mutually-exclusive
--tablet_filter
/--keyspaces_to_watch
featuresIf/when there is agreement on the approach I will add documentation for the new support and flag
Related Issue(s)
Resolves #15908
Checklist
Deployment Notes