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.

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

KAFKA-2061: Offer a --version flag to print the kafka version #639

Merged
merged 3 commits into from May 25, 2018

Conversation

Projects
None yet
6 participants
@sasakitoa
Contributor

sasakitoa commented Dec 8, 2015

Add version option to command line tools to print Kafka version

@@ -142,6 +142,13 @@ if [ -z "$KAFKA_JVM_PERFORMANCE_OPTS" ]; then
KAFKA_JVM_PERFORMANCE_OPTS="-server -XX:+UseG1GC -XX:MaxGCPauseMillis=20 -XX:InitiatingHeapOccupancyPercent=35 -XX:+DisableExplicitGC -Djava.awt.headless=true"
fi

# version option
for args in "$@" ; do
if [ "$args" == "--version" ]; then

This comment has been minimized.

@alindeman

alindeman Dec 19, 2015

Within a test expression like this, it's discouraged to use ==. A single equals tests if strings are identical:

-if [ "$args" == "--version" ]; then
+if [ "$args" = "--version" ]; then
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"
exit $?

This comment has been minimized.

@alindeman

alindeman Dec 19, 2015

The exec line above replaces the shell with the Java process. I don't think this line can be reached. I think it can be removed.

-exit $?
@alindeman

This comment has been minimized.

alindeman commented Dec 19, 2015

The other options to this command seem to use single dashes. Is a --version option going to confuse things?

@sasakitoa

This comment has been minimized.

Contributor

sasakitoa commented Dec 20, 2015

Thanks for comments, @alindeman

Within a test expression like this, it's discouraged to use == .

I don't think this line can be reached. I think it can be removed.

Thank you, You're right. I will correct these error, soon.
Sorry for easy mistake.

Is a --version option going to confuse things?

Yes it's, but in this ticket of JIRA ( https://issues.apache.org/jira/browse/KAFKA-2061 ) , --version is required.
Do you think "-version" is better then "--version" ?

@alindeman

This comment has been minimized.

alindeman commented Dec 20, 2015

Yes it's, but in this ticket of JIRA ( https://issues.apache.org/jira/browse/KAFKA-2061 ) , --version is required.
Do you think "-version" is better then "--version" ?

I think in the grand scheme of things --version is a better flag. I just wanted to bring up for discussion the fact that there are single - versions of long arguments right now too (e.g., -daemon, -name, etc.)

@sasakitoa

This comment has been minimized.

Contributor

sasakitoa commented Dec 21, 2015

Thanks, I got it.
I think long arguments with single dash is not good, too.

I use --version in this pull request.

@guozhangwang

This comment has been minimized.

Contributor

guozhangwang commented Dec 21, 2016

test this.

@guozhangwang

This comment has been minimized.

Contributor

guozhangwang commented Dec 21, 2016

@hachikuji Could you take a look at this PR?

@asfbot

This comment has been minimized.

asfbot commented Dec 21, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/340/
Test FAILed (JDK 7 and Scala 2.10).

@asfbot

This comment has been minimized.

asfbot commented Dec 21, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/342/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot

This comment has been minimized.

asfbot commented Dec 21, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/341/
Test PASSed (JDK 8 and Scala 2.12).

@hachikuji

This comment has been minimized.

Contributor

hachikuji commented Jan 4, 2017

I think most of our other commands use two hyphens for the arguments, so the current patch seems appropriate. I guess the downside of this approach is that the --version option won't show up in any of the --help comments, so users might not know it exists without trying it out. That said, it's such a common flag, perhaps that's fine?

@asfgit asfgit force-pushed the apache:trunk branch to 619fd7a May 3, 2017

@jeremydonahue

This comment has been minimized.

jeremydonahue commented Mar 14, 2018

Hi folks, is there anything I can do to help with this PR? I think a version command is important and I'd like to help.

Given that all of the arguments for kafka-run-class.sh are single-dash, I think the version command should be a single-dash despite the fact that most args in other tools are double-dash. It's really important to keep the args syntax for a tool consistent. If double-dash are the way to go, the tool (and probably all others that use single-dash) should be updated at the same time, with a note in the breaking changes release notes.

@sasakitoa

This comment has been minimized.

Contributor

sasakitoa commented Mar 14, 2018

I'm very sorry! I missed @hachikuji's comment.
Thank you for reviewing my PR, and replyed your comments as beflow.

That said, it's such a common flag, perhaps that's fine?

That's file in currently Pull Request.
I think --version command is common in most cases, not just Kafka.
Therefore, I think there is no problem that --version option is not shown in help.

I think the version command should be a single-dash despite the fact that most args in other tools are double-dash

Thank you for your comment.
I guess there are both opinions single-dash and double one.
I will try modify this PR to single-dash -version to be same other options.

I will update this PR soon, Could you please check new one?

@hachikuji

This comment has been minimized.

Contributor

hachikuji commented Mar 14, 2018

Forgot I had ever reviewed this. For what it's worth, I'm not sure -version is any more common than --version (for example, both python and git expect --version). Maybe we could support both?

@sasakitoa

This comment has been minimized.

Contributor

sasakitoa commented Mar 14, 2018

Maybe we could support both?

This idea seems good to me. I'll try.

@sasakitoa

This comment has been minimized.

Contributor

sasakitoa commented Mar 14, 2018

I modified to make enable to use both -version and --version .

@jeremydonahue

This comment has been minimized.

jeremydonahue commented Mar 14, 2018

Supporting both -version and --version is awesome, thanks! Should we do that for the rest of the args in this tool?

Also, it looks like there is no --help argument. Help is only printed if you run kafka-run-class.sh with no arguments. How do you feel about adding --help? I can submit a separate PR if that's more appropriate.

@sasakitoa

This comment has been minimized.

Contributor

sasakitoa commented Mar 15, 2018

I understood your comment means we should add --help option with same way.
(I'm sorry, if I misunderstood your comment)

I think we can use --help option with some commands w/o any changes such as kafka-topics.sh.
Most of commands have option parser and these are configured to accept --help options.

I proposed to add code to handle --version in kafka-run-class.sh because this is common for all commands. In addition to this, I thought it is too hard to add and maintain version option to option parsers of all commands.

@hachikuji

This comment has been minimized.

Contributor

hachikuji commented Mar 15, 2018

I think it's better to keep the scope of this limited to printing the version. Given some uncertainty over what argument we should use, and since this is a public API, I think we should do a short KIP to give others a chance to weigh in. I realize this PR has been open for a long time, but this is probably going to be the fastest way to make progress starting from here.

@jeremydonahue

This comment has been minimized.

jeremydonahue commented Mar 15, 2018

Sorry, I didn't realize that all of the other commands (eg. kafka-topics.sh) actually use kafka-run-class.sh. In that case it's definitely appropriate that kafka-run-class.sh doesn't have a help arg, as it would override the help arg of the classes being run. I do think this is a great place to put the --version command though, like you said, it's best to only maintain it in one place.

@sasakitoa

This comment has been minimized.

Contributor

sasakitoa commented Mar 16, 2018

Thank you for comments.

I think we should do a short KIP to give others a chance to weigh in.

OK, I'll try, but please give me some time because this is a first time to do KIP.

@hachikuji

This comment has been minimized.

Contributor

hachikuji commented Mar 16, 2018

@sasakitoa No hurry. I'm happy to take a look before you send it to the dev list if you like.

@sasakitoa

This comment has been minimized.

Contributor

sasakitoa commented Apr 4, 2018

I created KIP for this, cloud you give some advice and comments?
https://cwiki.apache.org/confluence/display/KAFKA/KIP-278+-+Add+version+option+to+Kafka%27s+commands
Thank you.

@sasakitoa sasakitoa force-pushed the sasakitoa:version_option branch from 206fe85 to f3876cd Apr 28, 2018

@sasakitoa

This comment has been minimized.

Contributor

sasakitoa commented May 24, 2018

The KIP related to this Pull Request was accepted.
Please see https://cwiki.apache.org/confluence/display/KAFKA/KIP-278+-+Add+version+option+to+Kafka%27s+commands for the detail.

@hachikuji

This comment has been minimized.

Contributor

hachikuji commented May 24, 2018

@sasakitoa Thanks for the KIP. Note the comments from @alindeman. Otherwise, it looks good.

@sasakitoa

This comment has been minimized.

Contributor

sasakitoa commented May 25, 2018

Thanks @hachikuji. I addressed comments from @alindeman

@hachikuji

LGTM. Thanks for the patch and incredible patience with the review.

@hachikuji hachikuji merged commit 440445e into apache:trunk May 25, 2018

2 checks passed

JDK 10 and Scala 2.12 SUCCESS 8809 tests run, 8 skipped, 0 failed.
Details
JDK 8 and Scala 2.11 SUCCESS 8809 tests run, 8 skipped, 0 failed.
Details

umesh9794 added a commit to umesh9794/kafka that referenced this pull request Jun 5, 2018

KAFKA-2061; Offer a --version flag to print the kafka version [KIP-27…
…8] (apache#639)

Reviewers: Andy Lindeman, Jeremy Donahue, Jason Gustafson <jason@confluent.io>

ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018

KAFKA-2061; Offer a --version flag to print the kafka version [KIP-27…
…8] (apache#639)

Reviewers: Andy Lindeman, Jeremy Donahue, Jason Gustafson <jason@confluent.io>

nimosunbit added a commit to sunbit-dev/kafka that referenced this pull request Nov 6, 2018

KAFKA-2061; Offer a --version flag to print the kafka version [KIP-27…
…8] (apache#639)

Reviewers: Andy Lindeman, Jeremy Donahue, Jason Gustafson <jason@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment