-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[ISSUE 9757][pulsar-admin] add cmd to get service url of the leader broker #9799
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice contribution!
/pulsarbot run-failure-checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
try { | ||
return pulsar().getLeaderElectionService().getCurrentLeader() | ||
.map(LeaderBroker::getServiceUrl) | ||
.orElse("None"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice contribution!
Now we will return null if we have no data. I think it is better to keep the same, otherwise the status returned by the Broker side will be more and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comment.
But maybe unclear semantics if we print "" on the console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In CMD, we can handle the case of null, but the exposed REST API should be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, i will take your advice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me.
I left a suggestion, thinking about future improvements to this API
Please take a look.
Once we publish an API we will have to maintain it basically forever
value = { | ||
@ApiResponse(code = 401, message = "Authentication required"), | ||
@ApiResponse(code = 403, message = "This operation requires super-user access") }) | ||
public String getLeaderBroker() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about returning a structure instead of a single String?
This way in the future we will be about to return more information .
If we return only one String we cannot extend this API in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestion.
What about returning LeaderBroker directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eolivelli Good suggestion.
Yes, I think you can use the class LeaderBroker directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to return LeaderBroker
.orElse(null); | ||
LeaderBroker leaderBroker = pulsar().getLeaderElectionService().getCurrentLeader() | ||
.orElseThrow(() -> new RestException(Status.NOT_FOUND, "Couldn't find leader broker")); | ||
BrokerInfo brokerInfo = new BrokerInfo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I create BrokerInfo which is the same as LeaderBroker, Because I can not access LeaderBroker in module pulsar-client-admin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eolivelli Please help to review this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect
+1
```shell | ||
$ pulsar-admin brokers leader-broker | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
If
pulsar-admin brokers leader-broker
has flags and descriptions, do not forget to add them to pulsar-admin docs. -
To allow users to have more info (flags and descriptions), it is better to add the URL of the
pulsar-admin brokers leader-broker
command on pulsar-admin. LikeFor more information, see [here](xxx).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your tip.
There is no more args for this command.
```java | ||
admin.brokers().getLeaderBroker() | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to add the Java admin URL to give more info to users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URL was contained in this doc.
The brokers method of the {@Inject: javadoc:PulsarAdmin:/admin/org/apache/pulsar/client/admin/PulsarAdmin.html} object in the Java API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your explanations. Sorry I do not get you. I mean we can add a link w/ the line pointed to this command? Like https://github.com/apache/pulsar/blob/master/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BrokersImpl.java#L80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for adding docs! I've commented, PTAL.
/pulsarbot run-failure-checks |
/pulsarbot run-failure-checks |
Thanks for the great work @linlinnn |
### Motivation To keep consistent with the [Java client](apache/pulsar#9799). ### Modifications Add `GetLeaderBroker` interface.
Fixes: #9757
Motivation
see #9757 add cmd to get service url of the leader broker.
Modifications
add cmd parameter "leader-broker" in CmdBrokers, and provide "/admin/v2/brokers/leaderBroker" web endpoint to get service url of the leader broker.
Verifying this change
Documentation
Does this pull request introduce a new feature? yes
If yes, how is the feature documented? ( JavaDocs )