-
Notifications
You must be signed in to change notification settings - Fork 11.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
[ROCKETMQ-278] Add clusterlist cmd by specified cluster #152
Conversation
1 similar comment
nice! |
clusterInfoSerializeWrapper.setBrokerAddrTable(this.brokerAddrTable); | ||
clusterInfoSerializeWrapper.setClusterAddrTable(this.clusterAddrTable); | ||
return clusterInfoSerializeWrapper.encode(); | ||
public byte[] getAllClusterInfo(String cluster) { |
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.
You change the signature of the public method here.
How about having another method -- public byte[] getAllClusterInfo()
? And you won't need calling getAllClusterInfo(null)
then too ;)
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 agree @shroman here
LGTM @zhouxinyu @vongosling |
@@ -1167,10 +1168,13 @@ public Properties getBrokerConfig(final String addr, final long timeoutMillis) | |||
throw new MQBrokerException(response.getCode(), response.getRemark()); | |||
} | |||
|
|||
public ClusterInfo getBrokerClusterInfo( | |||
public ClusterInfo getBrokerClusterInfo(String cluster, |
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.
how to insure the api compatibility
clusterInfoSerializeWrapper.setBrokerAddrTable(this.brokerAddrTable); | ||
clusterInfoSerializeWrapper.setClusterAddrTable(this.clusterAddrTable); | ||
return clusterInfoSerializeWrapper.encode(); | ||
public byte[] getAllClusterInfo(String cluster) { |
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 agree @shroman here
@@ -65,7 +65,7 @@ public void execute(final CommandLine commandLine, final Options options, | |||
try { | |||
defaultMQAdminExt.start(); | |||
if (commandLine.hasOption('c')) { | |||
ClusterInfo clusterInfo = defaultMQAdminExt.examineBrokerClusterInfo(); | |||
ClusterInfo clusterInfo = defaultMQAdminExt.examineBrokerClusterInfo(null); |
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.
null as parameter is not a good practice
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 change it in the next commit.
@lindzh would you please resolve the conflicts, I will merge it. |
@vongosling conflicts has been resolved. |
Too much merge request commits, could you mind using git pull --rebase to polish your pr ? |
@lindzh Thanks, I will pick up the real codes to merge into the develop branch. Expect you to keep your eyes on the RocketMQ community :-) |
What is the purpose of the change
When using mqadmin command clusterlist, it always display all cluster, I want to display specified cluster. JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-278.
Brief change log
Verifying this change
mqadmin clusterlist -c {slecified cluster}
to verify.Follow this checklist to help us incorporate your contribution quickly and easily: