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

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

Merged
merged 4 commits into from
Sep 7, 2023

Conversation

Technoboy-
Copy link
Contributor

@Technoboy- Technoboy- commented Jun 20, 2023

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
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 20, 2023
@JooHyukKim
Copy link
Contributor

JooHyukKim commented Jun 20, 2023

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-
Copy link
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
Copy link
Contributor

JooHyukKim commented Jun 25, 2023

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

EDIT : Done 👍🏻

@nodece
Copy link
Member

nodece commented Jun 25, 2023

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
Copy link
Contributor

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

@Technoboy-
Copy link
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
Copy link

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- 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
Copy link
Member

@RobertIndie RobertIndie left a comment

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-
Copy link
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
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs type/PIP
Projects
None yet
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