KAFKA-8131: Moved --version argument implementation from kafka-run-class.sh to CommandLineUtils to include documentation in command line help by soenkeliebau · Pull Request #6481 · apache/kafka

soenkeliebau · 2019-03-20T22:48:19Z

After some further investigation, I've taken the liberty of refactoring the implementation of the --version option and moved it into the default command options. This has the benefit of automatically including it in the usage output of the command line tools without having to actually touch any of them.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…ass.sh to CommandLineUtils to include documentation in command line help.

soenkeliebau · 2019-03-20T22:48:36Z

I've not yet included documentation and tests for this refactored code, as I'd like to discuss whether this makes sense first.

soenkeliebau · 2019-03-21T07:58:18Z

@hachikuji - you were one of the reviewers on the original addition of the parameter, could I perhaps trouble you to give this a quick once over?

…one place and could be inlined but might prove to be useful in other places as well. Refactored the class to be a bit more versatile.

soenkeliebau · 2019-04-08T20:54:51Z

@guozhangwang or @cmccabe maybe one of the two of you can have a quick look? ou I saw that both of you reviewed the addition of CommandDefaultOptions which I change in this...

guozhangwang

@soenkeliebau Thanks for the PR. Your approach looks clean and sound to me.

cc anyone of @ijuma @cmccabe @omkreddy @mumrah @gwenshap @rajinisivaram for another pair of eyes.

hachikuji

@soenkeliebau Apologies for the delay. It looks good to me, but let me check again in the morning when I'm a bit more sober. 😉

hachikuji

Thanks for the PR. Left a few comments.

hachikuji · 2019-04-10T02:50:35Z

  def printHelpAndExitIfNeeded(commandOpts: CommandDefaultOptions, message: String) = {
    if (isPrintHelpNeeded(commandOpts))
      printUsageAndDie(commandOpts.parser, message)
+    if (isPrintVersionNeeded(commandOpts))


Hmm.. I'm not sure this is called by all the commands that go through kafka-run-class.sh. For example, kafka.Kafka does not seem to hit this path.

I've been through the scripts in bin/ and looked at how many would be affected by this. There are three classes that use CommandLineUtils but don't hit this method: ReplicaVerificationTool, Kafka, StreamsResetter.
Coincidentally for StreamsResetter this means that the "--help" option, while listed in the output still results in an error.

The issue with these classes is, I think, that they internally do not use an object that extends CommandDefaultOptions - hence ignore those options completely.

The rest of the tools either call this method or handle argument parsing in a totally different way.

Not sure what the correct way forward is, to be honest, command line parsing seems a little all over the place in general, so the only place where this is truly global is indeed in kafka-run-class.sh, but that doesn't feel right either, as it is a completely undocumented on the command line that way.

If there only 3 exceptions, do you think it would be reasonable to implement --version separately for each of them? Perhaps in a follow-up we can consider how to refactor so that they all use CommandDefaultOptions. What do you think?

Hi @hachikuji
I am happy to implement those three separately for now.

In the interest of completion, handling those three would leave the following classes that do not support the version option (or help for that matter unless they implemented that separately):

ConnectDistributed - Doesn't use CommandLine

ConnectStandalone - Doesn't use CommandLine

ProducerPerformance - Uses argparse4j instead

VerifiableConsumer - Uses argparse4j instead

VerifiableProducer - Uses argparse4j instead

QuorumPeerMain - Zookeeper class

ZooKeeperMain - Zookeeper class

I'm not sure why a small subset of these uses argparse4j, I briefly looked around but couldn't find a larger scheme to adopt this over the CommandLineUtils, do you know anything more about that?

Should we extend these classes as well to honor the version flag? Should be simple-ish, but still requires adding an option to all parsers, unless we create the equivalent of the CommandDefaultOptions for argparse4j classes. But I'd say this is better suited for the follow-up PR you mentioned to standardize command line parsing.

So, my proposal woud be:
add very simple implementations to ConnectDistributed and ConnectStandalone as well, leave the rest be for now and look at the larger parsing picture in a follow up PR.

Fair?

Yeah, I'm not sure why they use argparse4j. Maybe just to make our lives harder. Your suggestion sounds fine to me. Would you mind filing JIRAs for the remaining work?

Happy to!
I'll kick off a thread on the mailing list first though, to ask around what people think we should do, personally, I'd like us to use just one command line parser, don't really care which one, but two is one too many for my liking.
But maybe there were reasons for the split.

I suspect there are no good reasons, but starting a discussion is a good idea. There is an ancient KIP which was attempting to consolidate these tools: https://cwiki.apache.org/confluence/display/KAFKA/KIP-14+-+Tools+Standardization. If you have any interest, it would really be helpful for someone to pick that up again. I think one of the reasons for the naming inconsistencies has been the fact that we are using different libraries.

Having looked some more at the two Connect classes, there is a small catch: everything we'd need to add the --version parameter to those classes is in the core Kafka project which Connect currently does not declare as a dependency. It seems a bit excessive to add that just for this purpose.
So I'll reword my suggestion to ignore these two for now and come up with an overall action plan first, before making any larger changes.

…hitting the relevant codepath in CommandLineUtils Marked options in CommandLineDefaultOptions as forHelp so that arguments that would usually be required are ignored if these are specified.

…formed.

hachikuji

Just a few additional comments

hachikuji

Thanks for the patch. LGTM. Before I merge, can you point me to the follow-up JIRAs? I'm (now) aware of https://issues.apache.org/jira/browse/KAFKA-8247. I think we also need to add --version to a few of the remaining tools like Connect?

soenkeliebau · 2019-04-25T20:59:29Z

Thanks for reviewing @hachikuji !
I've created KAFKA-8292 for the remaining five scripts and stared a discussion thread on the mailing list about reworking the entire parsing thing a little.
Is that sufficient?

soenkeliebau · 2019-05-05T08:34:08Z

ping @hachikuji

hachikuji · 2019-05-06T21:14:22Z

@soenkeliebau Thanks for the reminder. This one got lost in the noise. Let me run tests again to be safe and then I will merge.

hachikuji · 2019-05-06T21:14:30Z

retest this please

…s-hashcode * apache-github/trunk: KAFKA-8158: Add EntityType for Kafka RPC fields (apache#6503) MINOR: correctly parse version OffsetCommitResponse version < 3 KAFKA-8284: enable static membership on KStream (apache#6673) KAFKA-8304: Fix registration of Connect REST extensions (apache#6651) KAFKA-8275; Take throttling into account when choosing least loaded node (apache#6619) KAFKA-3522: Interactive Queries must return timestamped stores (apache#6661) MINOR: MetricsIntegrationTest should set StreamsConfig.STATE_DIR_CONFIG (apache#6687) MINOR: Remove unused field in `ListenerConnectionQuota` KAFKA-8131; Move --version implementation into CommandLineUtils (apache#6481) KAFKA-8056; Use automatic RPC generation for FindCoordinator (apache#6408) MINOR: Remove workarounds for lz4-java bug affecting byte buffers (apache#6679) KAFKA-7455: Support JmxTool to connect to a secured RMI port. (apache#5968) MINOR: Document improvement (apache#6682) MINOR: Fix ThrottledReplicaListValidator doc error. (apache#6537) KAFKA-8306; Initialize log end offset accurately when start offset is non-zero (apache#6652)

…he#6481) This patch refactors the implementation of the --version option and moves it into the default command options. This has the benefit of automatically including it in the usage output of the command line tools. Several tools had to be manually updated because they did not use the common options. Reviewers: Guozhang Wang <wangguoz@gmail.com>, Jason Gustafson <jason@confluent.io>

KAFKA-8131: Moved --version argument implementation from kafka-run-cl…

0cdf93b

…ass.sh to CommandLineUtils to include documentation in command line help.

soenkeliebau added 2 commits March 21, 2019 12:57

Reintroduced VersionInfo class, this is currently only being used in …

aba9ef8

…one place and could be inlined but might prove to be useful in other places as well. Refactored the class to be a bit more versatile.

Corrected whitespace f-up

1ff4e2e

guozhangwang approved these changes Apr 9, 2019

View reviewed changes

hachikuji reviewed Apr 10, 2019

View reviewed changes

soenkeliebau added 2 commits April 17, 2019 18:21

Added --version flag parsing to tools from core project that are not …

92db08c

…hitting the relevant codepath in CommandLineUtils Marked options in CommandLineDefaultOptions as forHelp so that arguments that would usually be required are ignored if these are specified.

Reverted accidental import "optimization" that Intellij helpfully per…

7ec6c23

…formed.

hachikuji reviewed Apr 23, 2019

View reviewed changes

Comment thread core/src/main/scala/kafka/Kafka.scala

Comment thread core/src/main/scala/kafka/Kafka.scala Outdated

Comment thread core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala Outdated

Comment thread core/src/main/scala/kafka/utils/CommandLineUtils.scala Outdated

Adressed comments from Jason.

049be2c

hachikuji approved these changes Apr 25, 2019

View reviewed changes

hachikuji merged commit 2bf153f into apache:trunk May 7, 2019

Conversation

soenkeliebau commented Mar 20, 2019

Committer Checklist (excluded from commit message)

Uh oh!

soenkeliebau commented Mar 20, 2019

Uh oh!

soenkeliebau commented Mar 21, 2019

Uh oh!

soenkeliebau commented Apr 8, 2019

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hachikuji Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soenkeliebau Apr 12, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji Apr 12, 2019

Choose a reason for hiding this comment

Uh oh!

soenkeliebau Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

soenkeliebau Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soenkeliebau Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

soenkeliebau commented Apr 25, 2019

Uh oh!

soenkeliebau commented May 5, 2019

Uh oh!

hachikuji commented May 6, 2019

Uh oh!

hachikuji commented May 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hachikuji Apr 10, 2019 •

edited

Loading

soenkeliebau Apr 16, 2019 •

edited

Loading

hachikuji Apr 16, 2019 •

edited

Loading