Make KeyVisualizer disabled by default #1645
Conversation
✅ Deploy Preview for tidb-dashboard canceled.
|
|
|
|
Welcome @smineyev81! It looks like this is your first PR to pingcap/tidb-dashboard 🎉 |
|
hi @smineyev81 , thanks for your contribution, but this change may affect other use cases, we need to investigate its influences, or choose a better way to not affect others as possible. |
@baurine , |
Yep, that's a better way. |
smineyev81
changed the title
@baurine, command line arg added. Please re-review |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1645 +/- ##
==========================================
+ Coverage 24.10% 26.27% +2.17%
==========================================
Files 173 97 -76
Lines 15715 10519 -5196
==========================================
- Hits 3788 2764 -1024
+ Misses 11643 7570 -4073
+ Partials 284 185 -99
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
cmd/tidb-dashboard/main.go
Outdated
| @@ -69,6 +69,7 @@ func NewCLIConfig() *DashboardCLIConfig { | |||
| flag.BoolVar(&cfg.CoreConfig.EnableExperimental, "experimental", cfg.CoreConfig.EnableExperimental, "allow experimental features") | |||
| flag.StringVar(&cfg.CoreConfig.FeatureVersion, "feature-version", cfg.CoreConfig.FeatureVersion, "target TiDB version for standalone mode") | |||
| flag.IntVar(&cfg.CoreConfig.NgmTimeout, "ngm-timeout", cfg.CoreConfig.NgmTimeout, "timeout secs for accessing the ngm API") | |||
| flag.BoolVar(&cfg.CoreConfig.EnableKeyVisualizer, "keyVisualizer", true, "enable/disable key visualizer(default: true)") | |||
There was a problem hiding this comment.
keyVisualizer is better to use keyviz as flag name to keep consistence with other flags.
There was a problem hiding this comment.
@baurine,
pingcap/tidb-operator#5536
tidb-operator PR that uses keyVisualizer name as command line arg already merged :(
changing it to keyviz would require another PR to tidb-operator
let me know if you still want me to rename it or it is ok to keep keyVisualizer
There was a problem hiding this comment.
It seems tidb-operator use the camel naming style, so keyVisualizer is fine, while tidb-dashboard use the xx-yy naming style, I think it's better to keep the same in one project.
There was a problem hiding this comment.
@baurine,
renamed to keyviz
please re-review
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baurine The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
KeyVisualizer sends very expensive queries to TiDB, so we had to disable it manually each time deploy Key Vizualizer https://tidb.support.pingcap.com/servicedesk/customer/portal/4/NAID-2058
What is changed and how does it work?
Makes KeyVisualizer disabled by default when tidb-dashboard is created first time