Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,6 @@ 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 versionOpt = parser.accepts("version", "Display Kafka version.")
var options: OptionSet = _
}
19 changes: 18 additions & 1 deletion core/src/main/scala/kafka/utils/CommandLineUtils.scala
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 = {
return commandOpts.options.has(commandOpts.versionOpt)
Comment thread
hachikuji marked this conversation as resolved.
Outdated
}

/**
* 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

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.

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.

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?

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?

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

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

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.

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
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})"
}
}