-
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
[api][pulsar-client]Add get version command for pulsar rest api, pulsar-admin, pulsar-client #9975
[api][pulsar-client]Add get version command for pulsar rest api, pulsar-admin, pulsar-client #9975
Conversation
/** | ||
* Pulsar version entry point. | ||
*/ | ||
public class PulsarVersionStarter { |
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.
Do we need this change?
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.
Yes, I found that other commands such as pulsar broker
, pulsar proxy
are a public class with a main
function, pulsar version
as a separate command, also needs to be a class with a main
function, otherwise, it needs to be a subcommand of other commands
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
Overall looks good.
I left a proposal, probably for a future enhancement
|
||
@Override | ||
void run() throws Exception { | ||
System.out.println( getAdmin().brokers().getVersion()); |
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 believe that we have a little usability problem here.
The user does not know which broker reported the version.
Proposals:
- return the address of the broker that reported the version (a BrokerInfo structure ?)
- add a (optional) parameter to specify the id of the broker that we want to query
In a Pulsar cluster usually you have the same version of the software, but this may not be the case during upgrades or in case you have special versions of Pulsar on some nodes.
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 agree with you, but I think the current version of the broker can be queried explicitly using the pulsar-admin brokers version
command, for example, the pulsar-admin brokers version
command will use the webServiceUrl
configuration in the client.conf
configuration file, and of course, the webServiceUrl
parameter can be used to specify the address of the broker to be queried by the command pulsar-admin --admin-url
, In a pulsar cluster upgrade, multiple versions of the broker will exist at the same time, and with the --admin-url
parameter, we can explicitly get the version of the specified broker
@tuteng thanks for your contribution. For docs side:
|
@codelipenghui PTAL |
Add docs w/ other changes here. |
…ar-admin, pulsar-client (#9975) Add the version command: Get broker version by rest API: ``` curl http://localhost:8080/admin/v2/brokers/version ``` Get broker version by `pulsar-admin`: ``` ./bin/pulsar-admin brokers version ``` Get broker version by the command `pulsar`: ``` ./bin/pulsar version ``` Get version of pulsar admin client: ``` ./bin/pulsar-admin -v ./bin/pulsra-admin --version ``` Get version of pulsar client: ``` ./bin/pulsar-client -v ./bin/pulsra-client --version (cherry picked from commit 5d85a2f)
(If this PR fixes a github issue, please add
Fixes #<xyz>
.)Fixes #9969
Motivation
Add the version command:
Get broker version by rest API:
Get broker version by
pulsar-admin
:Get broker version by the command
pulsar
:Get version of pulsar admin client:
Get version of pulsar client:
Modifications
pulsar
pulsar-admin
pulsar-client
Verifying this change
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation