KAFKA-14628: Move CommandLineUtils and CommandDefaultOptions shared classes#13131
Conversation
|
Take a look at DumpLogSegments to see how CommandDefaultOptions is used. It is a much better approach than building the script option list at the start of the main/execute method. In Java, you simply create a private static class that extends CommandDefaultOptions and put all parsing logic there. |
|
@clolov @vamossagar12 took your suggestions. Thanks. |
Thanks @fvaleri ! Couple of minor comments. This should be good to go after that! |
|
Thanks @fvaleri . LGTM. |
clolov
left a comment
There was a problem hiding this comment.
Actually, sorry, I still don't get it and I should have explained my question better. What I meant to ask is whether we can put all calls to
Exit.exit(1)only inCommandLineUtilsfunctions where we expect a termination of the program and not make explicit reference to it in other commands? For example, herehttps://github.com/apache/kafka/blob/0803659dc5b5e0aeae82b29e6ba4d07cb855bc1c/core/src/main/scala/kafka/admin/ZkSecurityMigrator.scala#L74
we have pushedExit.exit(1)down intoCommandLineUtils. And here
Reposting my question here as well, because I thought that continuing a resolved discussion will unresolve it, but apparently it does not :(
mimaison
left a comment
There was a problem hiding this comment.
Thanks for the PR, it looks good overall. I left a few suggestions.
@clolov it wasn't pushed down, that's the original logic. The code you are referring to in ZkSecurityMigrator.scala:103 is outdated. Now we are using ToolsUtils.printUsageAndExit in a few places for the reason expressed in the method comment. Please, pull latest changes and let me know. |
mimaison
left a comment
There was a problem hiding this comment.
LGTM, I'll wait for the CI to complete and then merge
…lasses These classes are required by most commands, so they must be migrated first. Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
c046e68 to
701a26b
Compare
|
Thanks for doing this. One thing I didn't understand is why the shared classes are in |
Good question. CommandLineUtils is mostly used by tools, but Kafka.scala has a dependency on it. In order to move these 2 shared classes to the tools module, we would need to add tools dependency to core. Is this correct? |
|
Got it, so it's useful for |
…apache#13131) Reviewers: Mickael Maison <mickael.maison@gmail.com>, Christo Lolov <christololov@gmail.com>, Sagar Rao <sagarmeansocean@gmail.com>
CommandLineUtilsandCommandDefaultOptionsare required by most commands, so they must be migrated first. This PR can be used as the base for other commands migration.