fix(throttler/demo): rename underscore flags to dash format, add manager tests#20033
Conversation
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
|
There was a problem hiding this comment.
Pull request overview
This PR aligns the throttler demo binary with Vitess’ dash-separated flag naming standard for v25 and adds direct unit coverage for two manager helper methods in the throttler package.
Changes:
- Renames the throttler demo’s remaining underscore-separated flags to dash-separated forms.
- Switches
registerDemoFlagsto the sharedutils.SetFlag*registration helpers used elsewhere in the codebase. - Adds unit tests for
Manager.Throttlers()andmanagerImpl.log().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
go/vt/throttler/manager_test.go |
Adds focused tests for throttler name listing and log lookup error handling. |
go/vt/throttler/demo/throttler_demo.go |
Renames demo CLI flags and uses the shared flag registration wrappers. |
|
@mattlord - please have a review when you get chance |
|
@mattlord - removed the entire demo directory as suggested, please have a review when you get chance |
mattlord
left a comment
There was a problem hiding this comment.
LGTM. Thank you, @ManthanNimodiya ! But let's please not open new PRs that offer/cover so little. It adds load to the shared CI systems and the maintainers. The Manager stuff was not even something listed in the noted issue and I don't see much practical value in these unit tests TBH.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20033 +/- ##
==========================================
+ Coverage 69.67% 72.22% +2.55%
==========================================
Files 1614 1346 -268
Lines 216793 204634 -12159
==========================================
- Hits 151044 147794 -3250
+ Misses 65749 56840 -8909
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@ManthanNimodiya And the DCO test is failing... I'm almost ready to close this as the review and CI overhead is IMO too much for a trivial change. But since we are already here, I will keep on with this one. |
…ger tests Signed-off-by: ManthanNimodiya <manthannimodiya989898@gmail.com>
Signed-off-by: ManthanNimodiya <manthannimodiya989898@gmail.com>
2f5016f to
75afe77
Compare
|
@mattlord, sorry for the extra noise on CI and your time. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
go/vt/throttler/demo/throttler_demo.go:1
- The PR description says this demo should keep existing functionality while renaming three flags, but this change deletes the entire throttler demo package. After the deletion,
go/vt/throttler/demono longer exists, so the local diagnostic binary and all of its behavior are removed rather than migrated to dash-form flag names.
Description
Three flag names in the throttler demo binary (
lag_update_interval,replica_degration_interval,replica_degration_duration) still used underscore separators, conflicting with the project-wide standard established in v25. This PR renames them and migrates all flag registrations inregisterDemoFlagstoutils.SetFlag*wrappers to match the pattern used across every other Vitess binary.Also adds
TestManager_ThrottlersandTestManager_Logtomanager_test.go— both methods existed without any dedicated test coverage.Related Issue(s)
Fixes part of #17880 (flag renaming: replace underscores with dashes)
Related to #14931 (improve unit test coverage)
Checklist
Deployment Notes
No deployment impact. The throttler demo is a local diagnostic binary, not a production component. The flag rename is intentional — v25 removes underscore flag support.
AI Disclosure
Issue scope was analysed thoroughly before implementing this fix.