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.

Sign up for GitHub

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

Jump to bottom

cluster: Remove duplicate display of dashboard URLs #2502

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cluster: Remove duplicate display of dashboard URLs #2502

wants to merge 1 commit into from
+4 −2

Conversation

dveeden
Copy link
Contributor

@dveeden dveeden commented Jan 21, 2025 •
edited
Loading

What problem does this PR solve?

Note the two lines with Dashboard URL here:

$ tiup cluster display dvetest1 
Cluster type:       tidb
Cluster name:       dvetest1
Cluster version:    v8.5.0
Deploy user:        tidb
SSH type:           builtin
Dashboard URL:      http://192.168.122.196:2379/dashboard
Dashboard URLs:     http://192.168.122.196:2379/dashboard
Grafana URL:        http://192.168.122.196:3000
ID                     Role        Host             Ports        OS/Arch       Status   Data Dir                    Deploy Dir
--                     ----        ----             -----        -------       ------   --------                    ----------
192.168.122.196:3000   grafana     192.168.122.196  3000         linux/x86_64  Up       -                           /tidb-deploy/grafana-3000
192.168.122.196:2379   pd          192.168.122.196  2379/2380    linux/x86_64  Up|L|UI  /tidb-data/pd-2379          /tidb-deploy/pd-2379
192.168.122.196:9090   prometheus  192.168.122.196  9090/12020   linux/x86_64  Up       /tidb-data/prometheus-9090  /tidb-deploy/prometheus-9090
192.168.122.196:4000   tidb        192.168.122.196  4000/10080   linux/x86_64  Up       -                           /tidb-deploy/tidb-4000
192.168.122.196:20160  tikv        192.168.122.196  20160/20180  linux/x86_64  Up       /tidb-data/tikv-20160       /tidb-deploy/tikv-20160
Total nodes: 5

Related: #2472

What is changed and how it works?

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Now only one line in the output: Dashboard URL: http://192.168.122.196:2379/dashboard

Release notes:

NONE

@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 21, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign xhebox for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot requested a review from srstack January 21, 2025 11:05
@ti-chi-bot ti-chi-bot bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 21, 2025
@dveeden
Copy link
Contributor Author

dveeden commented Jan 21, 2025

/cc @xhebox

@ti-chi-bot ti-chi-bot bot requested a review from xhebox January 21, 2025 11:06
xhebox
xhebox reviewed Jan 22, 2025
View reviewed changes
pkg/cluster/manager/display.go
@@ -844,9 +844,5 @@ func (m *Manager) displayDashboards(ctx context.Context, t *spec.Specification,
}
}

if m.logger.GetDisplayMode() != logprinter.DisplayModeJSON || j == nil {
fmt.Printf("Dashboard URLs: %s\n", strings.Join(dashboardAddrs, ","))
Copy link
Collaborator

@xhebox xhebox Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is a new function that can display multiple dashboard address(dashboard_servers). So it is invalid to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe there should be an if len(dashboardAddrs) > 1 around it instead? And then also a if len(dashboardAddrs) == 1 before line 836 ?

Also, does it use the right scheme if there are multiple?

Copy link
Contributor Author

@dveeden dveeden Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've updated this to check for the length of dashboardAddrs.

PTAL @xhebox

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, LGTM. But I don't know if it is wanted or not.

It is a new feature but Dashboard URL: xxx is an old feature. There are compatibility issues.

This feature is asked by the team of @sydpz, PTAL.

Copy link
Member

Choose a reason for hiding this comment

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

How about displaying multiple dashboard URLs in Dashboard URL?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about displaying multiple dashboard URLs in Dashboard URL?

I've considered, and I think it will break existed applications. For example, the team of TEM does rely on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the output or in the json structure?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Output, of course. I did not modify json structure, iirc.

Copy link
Contributor Author

@dveeden dveeden Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't humans rely on the human readable output and programs on the machine readable json? Maybe even over a REST interface instead of by calling a command.

Shouldn't TEM be modified to follow the output of tiup?

Another option would be to do something like git with Porcelain and Plumbing. Or something like --output v1...

For now I'm also ok with just closing this. It isn't that important.

@dveeden dveeden force-pushed the cluster_display_dashboard_urls branch from 7e2a86e to 949dfa5 Compare January 22, 2025 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breezewish breezewish breezewish left review comments

@xhebox xhebox xhebox left review comments

@srstack srstack Awaiting requested review from srstack

Assignees
No one assigned
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dveeden @breezewish @xhebox