Skip to content

[improve][pip] PIP-277: Add current option in the Clusters list cmd#20614

Merged
Technoboy- merged 4 commits into
apache:masterfrom
Technoboy-:pip-277
Sep 7, 2023
Merged

[improve][pip] PIP-277: Add current option in the Clusters list cmd#20614
Technoboy- merged 4 commits into
apache:masterfrom
Technoboy-:pip-277

Conversation

@Technoboy-

@Technoboy- Technoboy- commented Jun 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #20581

Motivation

After configuring the geo-replication on Pulsar clusters, the clusters list API will return multiple clusters, including the local Pulsar cluster and remote clusters like

bin/pulsar-admin clusters list
us-west
us-east
us-cent

But in this return, you can't distinguish the local and the remote cluster. When you need to remove the geo-replication configuration, it will be hard to decide which cluster should be removed on replicated tenants and namespaces unless you record the cluster information.

Modification

Add --current option in the cluster list cmd and mark the current cluster with (*)

bin/pulsar-admin clusters list --current
us-west(*)
us-east
us-cent

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@Technoboy- Technoboy- self-assigned this Jun 20, 2023
@Technoboy- Technoboy- added this to the 3.1.0 milestone Jun 20, 2023
@Technoboy- Technoboy- mentioned this pull request Jun 19, 2023
Closed
4 tasks
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jun 20, 2023
@JooHyukKim

JooHyukKim commented Jun 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Add local flag to distinguish clusters

Would you consider adding remote flag also, @Technoboy- ? 🙂 It seems like we are sort of allowing users to assume what the ones without local flag are (are called by the community) 🤔 Plus users will benefit from not having to think about what non-local clusters are called.

Possible use-case would be someone automates counting how many remotes and clusters are there in list via shellscript, or
simple String parser. It will be more convinient to group by local vs remote, rather than local vs ""

@Technoboy- Technoboy- mentioned this pull request Jun 25, 2023
Merged
4 tasks
@Technoboy-

Technoboy- commented Jun 25, 2023

Copy link
Copy Markdown
Contributor Author

Add local flag to distinguish clusters

Would you consider adding remote flag also, @Technoboy- ? 🙂 It seems like we are sort of allowing users to assume what the ones without local flag are (are called by the community) 🤔 Plus users will benefit from not having to think about what non-local clusters are called.

Possible use-case would be someone automates counting how many remotes and clusters are there in list via shellscript, or simple String parser. It will be more convinient to group by local vs remote, rather than local vs ""

yes, sure. Could you also send these ideas in this mail ?

@JooHyukKim

JooHyukKim commented Jun 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@Technoboy- For sure, will do.👍🏻 But I am not sure if I have access 🤔.

EDIT : Done 👍🏻

@nodece

nodece commented Jun 25, 2023

Copy link
Copy Markdown
Member

It will be more convinient to group by local vs remote

I think this is a good idea to distinguish between local and remote types.

# list remote clusters
bin/pulsar-admin clusters list --remote
# list local clustes
bin/pulsar-admin clusters list --local
# list all clusters
bin/pulsar-admin clusters list

@JooHyukKim

JooHyukKim commented Jun 25, 2023

Copy link
Copy Markdown
Contributor

+1 on @nodece 's proposal wrt --local and --remote flag 👍🏻

@Technoboy-

Technoboy- commented Jun 25, 2023

Copy link
Copy Markdown
Contributor Author

It will be more convinient to group by local vs remote

I think this is a good idea to distinguish between local and remote types.

# list remote clusters
bin/pulsar-admin clusters list --remote
# list local clustes
bin/pulsar-admin clusters list --local
# list all clusters
bin/pulsar-admin clusters list

Good idea, could you also discuss this in this mail ?

@Technoboy- Technoboy- changed the title [pip][design] PIP-277: Clusters list API return clusters with local flag [pip][design] PIP-277: Add --current option in the Clusters list API Jun 27, 2023
@Technoboy- Technoboy- changed the title [pip][design] PIP-277: Add --current option in the Clusters list API [pip][design] PIP-277: Add --current option in the Clusters list cmd Jun 27, 2023
@github-actions

github-actions Bot commented Jul 28, 2023

Copy link
Copy Markdown

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions Bot added the Stale label Jul 28, 2023
@Technoboy- Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023
@Technoboy- Technoboy- changed the title [pip][design] PIP-277: Add --current option in the Clusters list cmd [pip][design] PIP-277: Add current option in the Clusters list cmd Sep 6, 2023
@Technoboy- Technoboy- mentioned this pull request Sep 6, 2023
Merged
4 tasks
@Technoboy- Technoboy- changed the title [pip][design] PIP-277: Add current option in the Clusters list cmd [improve][pip] PIP-277: Add current option in the Clusters list cmd Sep 7, 2023
@Technoboy- Technoboy- removed the Stale label Sep 7, 2023
RobertIndie
RobertIndie reviewed Sep 7, 2023
View reviewed changes

@RobertIndie RobertIndie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did you move pip-274.md? I see that the same change also appears in https://github.com/apache/pulsar/pull/20993/files.

@Technoboy-

Technoboy- commented Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

Why did you move pip-274.md? I see that the same change also appears in https://github.com/apache/pulsar/pull/20993/files.

pip-274.md should rename to pip-276.md. Then in #20993, I have done it. So no need to update in this pr

@Technoboy- Technoboy- merged commit d890432 into apache:master Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RobertIndie RobertIndie RobertIndie approved these changes

@Demogorgon314 Demogorgon314 Demogorgon314 approved these changes

@mattisonchao mattisonchao mattisonchao approved these changes

Assignees

@Technoboy- Technoboy-

Labels

doc-not-needed Your PR changes do not impact docs type/PIP

Projects

None yet

Milestone

3.2.0

Development

Successfully merging this pull request may close these issues.

[Improve][Admin API] Clusters list API return clusters with local and remote flag

6 participants

@Technoboy- @JooHyukKim @nodece @RobertIndie @Demogorgon314 @mattisonchao