KAFKA-14575: Move ClusterTool to tools module#13080
Conversation
fvaleri
left a comment
fvaleri
left a comment
There was a problem hiding this comment.
I tried your changes on a running cluster and they work fine.
Left few comments.
Thanks.
| .help("A list of host/port pairs to use for establishing the connection to the kafka cluster."); | ||
| subpparser.addArgument("--config", "-c") | ||
| .action(store()) | ||
| .help("A property file containing configs to passed to AdminClient."); |
There was a problem hiding this comment.
| .help("A property file containing configs to passed to AdminClient."); | |
| .help("A property file containing custom configs for the AdminClient."); |
There was a problem hiding this comment.
Thanks, I've reworded that message
| Exit.exit(mainNoExit(args)); | ||
| } | ||
|
|
||
| static int mainNoExit(String... args) { |
There was a problem hiding this comment.
Why not simply embed this logic inside the main method to have one less stack frame?
There was a problem hiding this comment.
The idea was to make it like MetadataQuorumCommand. In the future we will be able to put this method in a Command interface or utils class.
There was a problem hiding this comment.
I like the idea of the command interface. In that case, we should add the suffix *Command to all implementing classes (e.g. ClusterToolCommand).
There was a problem hiding this comment.
Yes. I was thinking of doing that after the Scala to Java conversion.
|
LGTM. Thanks. |
dengziming
left a comment
dengziming
left a comment
There was a problem hiding this comment.
LGTM, I find that we didn't have a 1bin/windows/kafka-cluster.bat1 currently, how about added it here as a connivence.
|
@dengziming I opened https://issues.apache.org/jira/browse/KAFKA-14614 to look at the missing script for Windows |
Reviewers: dengziming <dengziming1993@gmail.com>, Federico Valeri <fedevaleri@gmail.com>
3 participants
Committer Checklist (excluded from commit message)