Skip to content
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

Refine the error message of monitor port conflict #966

Merged
merged 4 commits into from
Dec 8, 2020

Conversation

JaySon-Huang
Copy link
Contributor

Signed-off-by: JaySon-Huang tshent@qq.com

What problem does this PR solve?

Fix #965. Refine the error message of monitor port conflict.

What is changed and how it works?

Use spec.RoleMonitor as the component name instead of the instance name.

Check List

Tests

  • Unit test

Code changes

  • N/A

Side effects

  • N/A

Related changes

  • Need to cherry-pick to the release branch

Release notes:

Refine the error message of monitor port conflict

Signed-off-by: JaySon-Huang <tshent@qq.com>
@ti-chi-bot ti-chi-bot requested a review from july2993 December 3, 2020 11:45
@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 3, 2020
@codecov-io
Copy link

codecov-io commented Dec 3, 2020

Codecov Report

Merging #966 (5319936) into master (3bf3ba8) will decrease coverage by 4.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #966      +/-   ##
==========================================
- Coverage   55.78%   51.71%   -4.08%     
==========================================
  Files         263      263              
  Lines       19522    19529       +7     
==========================================
- Hits        10891    10100     -791     
- Misses       6903     7798     +895     
+ Partials     1728     1631      -97     
Flag Coverage Δ
cluster 37.69% <48.14%> (-5.63%) ⬇️
dm 24.27% <25.92%> (-0.03%) ⬇️
integrate 45.85% <48.14%> (-4.15%) ⬇️
playground 20.26% <ø> (ø)
tiup 16.79% <0.00%> (-0.02%) ⬇️
unittest 23.09% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/cluster/spec/validate.go 92.93% <100.00%> (+0.10%) ⬆️
components/cluster/command/check.go 6.37% <0.00%> (-72.82%) ⬇️
pkg/cluster/task/limits.go 0.00% <0.00%> (-68.75%) ⬇️
pkg/cluster/task/sysctl.go 0.00% <0.00%> (-66.67%) ⬇️
components/cluster/command/audit.go 27.27% <0.00%> (-54.55%) ⬇️
pkg/cluster/operation/check.go 0.00% <0.00%> (-51.95%) ⬇️
pkg/cluster/task/rmdir.go 0.00% <0.00%> (-50.00%) ⬇️
pkg/cluster/operation/operation.go 34.78% <0.00%> (-43.48%) ⬇️
components/cluster/command/rename.go 25.00% <0.00%> (-41.67%) ⬇️
components/cluster/command/stop.go 38.46% <0.00%> (-30.77%) ⬇️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bf3ba8...5319936. Read the comment docs.

port: port,
clusterName: name,
componentName: inst.ComponentName(),
host: inst.GetHost(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, I didn't quite get the point of adding a new field, we could use inst.GetHost() to get the value anytime.

Copy link
Contributor Author

@JaySon-Huang JaySon-Huang Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to make the message contains in Entry clear and eliminate the ambiguity. If there exist fields port, componentName along with inst, should I get the port from inst.GetPort() or get the name from inst.ComponentName()?
However, keeping inst instead of adding host is more flexible.

I can keep inst instead of adding host if you insist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I would still suggest using inst as it carries more information.

pkg/cluster/spec/validate_test.go Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 8, 2020
@AstroProfundis
Copy link
Contributor

/merge

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 8, 2020
@ti-chi-bot
Copy link
Member

Can merge label has been added.

Git tree hash: 9fd504c

@ti-chi-bot ti-chi-bot merged commit e534e62 into pingcap:master Dec 8, 2020
@JaySon-Huang JaySon-Huang deleted the fix_port_conflict_comp_name branch December 8, 2020 05:44
@AstroProfundis AstroProfundis added the category/monitoring Categorizes issue or PR related to monitoring components. label Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/monitoring Categorizes issue or PR related to monitoring components. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The error message of monitor port conflict is misleading
5 participants