Skip to content

KAFKA-8131: Moved --version argument implementation from kafka-run-class.sh to CommandLineUtils to include documentation in command line help #6481

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.

Sign up for GitHub

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

Merged
hachikuji merged 6 commits into from
May 7, 2019
Merged

KAFKA-8131: Moved --version argument implementation from kafka-run-class.sh to CommandLineUtils to include documentation in command line help #6481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0cdf93b
KAFKA-8131: Moved --version argument implementation from kafka-run-cl…
soenkeliebau Mar 20, 2019
aba9ef8
Reintroduced VersionInfo class, this is currently only being used in …
soenkeliebau Mar 21, 2019
1ff4e2e
Corrected whitespace f-up
soenkeliebau Mar 21, 2019
92db08c
Added --version flag parsing to tools from core project that are not …
soenkeliebau Apr 17, 2019
7ec6c23
Reverted accidental import "optimization" that Intellij helpfully per…
soenkeliebau Apr 17, 2019
049be2c
Adressed comments from Jason.
soenkeliebau Apr 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions bin/kafka-run-class.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,13 +238,6 @@ if [ -z "$KAFKA_JVM_PERFORMANCE_OPTS" ]; then
KAFKA_JVM_PERFORMANCE_OPTS="-server -XX:+UseG1GC -XX:MaxGCPauseMillis=20 -XX:InitiatingHeapOccupancyPercent=35 -XX:+ExplicitGCInvokesConcurrent -Djava.awt.headless=true"
fi

# version option
for args in "$@" ; do
if [ "$args" = "--version" ]; then
exec $JAVA $KAFKA_HEAP_OPTS $KAFKA_JVM_PERFORMANCE_OPTS $KAFKA_GC_LOG_OPTS $KAFKA_JMX_OPTS $KAFKA_LOG4J_OPTS -cp $CLASSPATH $KAFKA_OPTS "kafka.utils.VersionInfo"
fi
done

while [ $# -gt 0 ]; do
COMMAND=$1
case $COMMAND in
Expand Down
11 changes: 10 additions & 1 deletion core/src/main/scala/kafka/Kafka.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,20 @@ object Kafka extends Logging {
val overrideOpt = optionParser.accepts("override", "Optional property that should override values set in server.properties file")
.withRequiredArg()
.ofType(classOf[String])
// This is just to make the parameter show up in the help output, we are not actually using this due the
// fact that this class ignores the first parameter which is interpreted as positional and mandatory
// but would not be mandatory if --version is specified
// This is a bit of an ugly crutch till we get a chance to rework the entire command line parsing
val versionOpt = optionParser.accepts("version", "Print version information and exit.")

if (args.length == 0) {
if (args.length == 0 || args.contains("--help")) {
CommandLineUtils.printUsageAndDie(optionParser, "USAGE: java [options] %s server.properties [--override property=value]*".format(classOf[KafkaServer].getSimpleName()))
}

if (args.contains("--version")) {
Comment thread
hachikuji marked this conversation as resolved.
CommandLineUtils.printVersionAndDie()
}

val props = Utils.loadProps(args(0))

if (args.length > 1) {
Expand Down
13 changes: 10 additions & 3 deletions core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,18 @@ object ReplicaVerificationTool extends Logging {
.describedAs("ms")
.ofType(classOf[java.lang.Long])
.defaultsTo(30 * 1000L)
val helpOpt = parser.accepts("help", "Print usage information.").forHelp()
val versionOpt = parser.accepts("version", "Print version information and exit.").forHelp()

if (args.length == 0)
val options = parser.parse(args: _*)

if (args.length == 0 || options.has(helpOpt)) {
CommandLineUtils.printUsageAndDie(parser, "Validate that all replicas for a set of topics have the same data.")
}

val options = parser.parse(args: _*)
if (options.has(versionOpt)) {
CommandLineUtils.printVersionAndDie()
}
CommandLineUtils.checkRequiredArgs(parser, options, brokerListOpt)

val regex = options.valueOf(topicWhiteListOpt)
Expand Down Expand Up @@ -177,7 +184,7 @@ object ReplicaVerificationTool extends Logging {
// create all replica fetcher threads
val verificationBrokerId = brokerToTopicPartitions.head._1
val counter = new AtomicInteger(0)
val fetcherThreads: Iterable[ReplicaFetcher] = brokerToTopicPartitions.map { case (brokerId, topicPartitions) =>
val fetcherThreads = brokerToTopicPartitions.map { case (brokerId, topicPartitions) =>
new ReplicaFetcher(name = s"ReplicaFetcher-$brokerId",
sourceBroker = brokerInfo(brokerId),
topicPartitions = topicPartitions,
Expand Down
12 changes: 8 additions & 4 deletions core/src/main/scala/kafka/tools/StreamsResetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ public class StreamsResetter {
private static OptionSpec<String> fromFileOption;
private static OptionSpec<Long> shiftByOption;
private static OptionSpecBuilder dryRunOption;
private static OptionSpecBuilder helpOption;
private static OptionSpec helpOption;
private static OptionSpec versionOption;
private static OptionSpecBuilder executeOption;
private static OptionSpec<String> commandConfigOption;

Expand Down Expand Up @@ -238,7 +239,8 @@ private void parseArguments(final String[] args) throws IOException {
.describedAs("file name");
executeOption = optionParser.accepts("execute", "Execute the command.");
dryRunOption = optionParser.accepts("dry-run", "Display the actions that would be performed without executing the reset commands.");
helpOption = optionParser.accepts("help", "Print usage information.");
helpOption = optionParser.accepts("help", "Print usage information.").forHelp();
versionOption = optionParser.accepts("version", "Print version information and exit.").forHelp();

// TODO: deprecated in 1.0; can be removed eventually: https://issues.apache.org/jira/browse/KAFKA-7606
optionParser.accepts("zookeeper", "Zookeeper option is deprecated by bootstrap.servers, as the reset tool would no longer access Zookeeper directly.");
Expand All @@ -248,9 +250,11 @@ private void parseArguments(final String[] args) throws IOException {
if (args.length == 0 || options.has(helpOption)) {
CommandLineUtils.printUsageAndDie(optionParser, usage);
}
if (options.has(versionOption)) {
CommandLineUtils.printVersionAndDie();
}
} catch (final OptionException e) {
printHelp(optionParser);
throw e;
CommandLineUtils.printUsageAndDie(optionParser, e.getMessage());
}

if (options.has(executeOption) && options.has(dryRunOption)) {
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/scala/kafka/utils/CommandDefaultOptions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import joptsimple.{OptionParser, OptionSet}

abstract class CommandDefaultOptions(val args: Array[String], allowCommandOptionAbbreviation: Boolean = false) {
val parser = new OptionParser(allowCommandOptionAbbreviation)
val helpOpt = parser.accepts("help", "Print usage information.")
val helpOpt = parser.accepts("help", "Print usage information.").forHelp()
val versionOpt = parser.accepts("version", "Display Kafka version.").forHelp()
var options: OptionSet = _
}
19 changes: 18 additions & 1 deletion core/src/main/scala/kafka/utils/CommandLineUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.Properties

import joptsimple.{OptionParser, OptionSet, OptionSpec}
import org.apache.kafka.common.utils.AppInfoParser

import scala.collection.Set

Expand All @@ -36,16 +37,27 @@ object CommandLineUtils extends Logging {
return commandOpts.args.length == 0 || commandOpts.options.has(commandOpts.helpOpt)
}

def isPrintVersionNeeded(commandOpts: CommandDefaultOptions): Boolean = {
commandOpts.options.has(commandOpts.versionOpt)
}

/**
* Check and print help message if there is no options or `--help` option
* from command line
* from command line, if `--version` is specified on the command line
* print version information and exit.
* NOTE: The function name is not strictly speaking correct anymore
* as it also checks whether the version needs to be printed, but
* refactoring this would have meant changing all command line tools
* and unnecessarily increased the blast radius of this change.
*
* @param commandOpts Acceptable options for a command
* @param message Message to display on successful check
*/
def printHelpAndExitIfNeeded(commandOpts: CommandDefaultOptions, message: String) = {
if (isPrintHelpNeeded(commandOpts))
printUsageAndDie(commandOpts.parser, message)
if (isPrintVersionNeeded(commandOpts))

@hachikuji hachikuji Apr 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@soenkeliebau soenkeliebau Apr 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hachikuji hachikuji Apr 12, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@soenkeliebau soenkeliebau Apr 15, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@hachikuji hachikuji Apr 15, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@soenkeliebau soenkeliebau Apr 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hachikuji hachikuji Apr 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@soenkeliebau soenkeliebau Apr 17, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

printVersionAndDie()
}

/**
Expand Down Expand Up @@ -91,6 +103,11 @@ object CommandLineUtils extends Logging {
Exit.exit(1, Some(message))
}

def printVersionAndDie(): Nothing = {
System.out.println(VersionInfo.getVersionString)
Exit.exit(0)
}

/**
* Parse key-value pairs in the form key=value
* value may contain equals sign
Expand Down
16 changes: 13 additions & 3 deletions core/src/main/scala/kafka/utils/VersionInfo.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,19 @@ import org.apache.kafka.common.utils.AppInfoParser
object VersionInfo {

def main(args: Array[String]) {
val version = AppInfoParser.getVersion
val commitId = AppInfoParser.getCommitId
System.out.println(s"${version} (Commit:${commitId})")
System.out.println(getVersionString)
System.exit(0)
}

def getVersion: String = {
AppInfoParser.getVersion
}

def getCommit: String = {
AppInfoParser.getCommitId
}

def getVersionString: String = {
s"${getVersion} (Commit:${getCommit})"
}
}