KAFKA-4457. Add BrokerVersionCommand by cmccabe · Pull Request #2184 · apache/kafka

cmccabe · 2016-11-28T23:13:01Z

No description provided.

ewencp · 2016-12-31T04:15:24Z

This seems non-standard wrt the format of config options. This should probably be - separated words.

ewencp · 2016-12-31T04:32:36Z

With these tools, should we consider better ways of providing JSON output? In particular, could we possibly provide output that uses Jackson to dump output and only depends on it for tools (i.e. without requiring it for everything in core)?

That would be nice. Probably means that we'd want the tool to be in Java in the tools subproject. We would then not be able to use AdminClient.scala. Maybe we can do that as a subsequent JIRA as it will be easier once there is a Java AdminClient.

ewencp · 2016-12-31T05:12:37Z

Working locally with this patch, I'm still getting an error trying to test these new commands. Seems the API versions don't work -- any idea what's going wrong?

java.lang.IllegalStateException: Attempt to send API Versions request to node 0 which is not ready. at org.apache.kafka.clients.NetworkClient.doSend(NetworkClient.java:309) at org.apache.kafka.clients.NetworkClient.send(NetworkClient.java:296) at org.apache.kafka.clients.consumer.internals.ConsumerNetworkClient.trySend(ConsumerNetworkClient.java:398) at org.apache.kafka.clients.consumer.internals.ConsumerNetworkClient.poll(ConsumerNetworkClient.java:255) at org.apache.kafka.clients.consumer.internals.ConsumerNetworkClient.poll(ConsumerNetworkClient.java:181) at kafka.admin.AdminClient.kafka$admin$AdminClient$$send(AdminClient.scala:49) at kafka.admin.AdminClient.getApiVersions(AdminClient.scala:83) at kafka.admin.AdminClient$$anonfun$listAllBrokerVersionInfo$1.liftedTree2$1(AdminClient.scala:166) at kafka.admin.AdminClient$$anonfun$listAllBrokerVersionInfo$1.apply(AdminClient.scala:165) at kafka.admin.AdminClient$$anonfun$listAllBrokerVersionInfo$1.apply(AdminClient.scala:162) at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244) at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244) at scala.collection.immutable.List.foreach(List.scala:318) at scala.collection.TraversableLike$class.map(TraversableLike.scala:244) at scala.collection.AbstractTraversable.map(Traversable.scala:105) at kafka.admin.AdminClient.listAllBrokerVersionInfo(AdminClient.scala:162) at kafka.admin.BrokerVersionCommand$.main(BrokerVersionCommand.scala:37) at kafka.admin.BrokerVersionCommand.main(BrokerVersionCommand.scala)

Seems like merging KAFKA-3600 has broken this PR.

asfbot · 2017-01-05T22:23:45Z

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

asfbot · 2017-01-05T23:28:08Z

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

asfbot · 2017-01-05T23:41:47Z

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

asfbot · 2017-01-12T00:27:45Z

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

asfbot · 2017-01-12T01:07:32Z

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

asfbot · 2017-01-12T01:10:27Z

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

apurvam

Mostly looks fine. But I think the format of the output needs to be improved for it to be useful.

apurvam · 2017-01-12T01:47:12Z

This output seems a bit illegible. Everything is on a single line:

localhost:9092 (id: 0 rack: null) -> {Produce(0): 0 to 2 [usable: 2], Fetch(1): 0 to 3 [usable: 3], Offsets(2): 0 to 1 [usable: 1], Metadata(3): 0 to 2 [usable: 2], LeaderAndIsr(4): 0 [usable: 0], StopReplica(5): 0 [usable: 0], UpdateMetadata(6): 0 to 2 [usable: 2], ControlledShutdown(7): 1 [usable: 1], OffsetCommit(8): 0 to 2 [usable: 2], OffsetFetch(9): 0 to 1 [usable: 1], GroupCoordinator(10): 0 [usable: 0], JoinGroup(11): 0 to 1 [usable: 1], Heartbeat(12): 0 [usable: 0], LeaveGroup(13): 0 [usable: 0], SyncGroup(14): 0 [usable: 0], DescribeGroups(15): 0 [usable: 0], ListGroups(16): 0 [usable: 0], SaslHandshake(17): 0 [usable: 0], ApiVersions(18): 0 [usable: 0], CreateTopics(19): 0 [usable: 0], DeleteTopics(20): 0 [usable: 0]}

That isn't json, and is not very human readable either. Shall we at least have one line per command?

OK, we can put each API key on its own line and use some indentation.

asfbot · 2017-01-12T23:33:14Z

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

asfbot · 2017-01-12T23:37:07Z

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

asfbot · 2017-01-12T23:37:20Z

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

apurvam · 2017-01-13T18:16:25Z

so, if the server is down, or if the request times out, we will still get an output with all APIS showing 'Unsupported'. IF we didn't get a response, it might be better to just print the error (timeout, connection refused, etc.), and return.

Fair enough. Let's use a scala.Try to represent the fact that we might get an exception here.

apurvam

Overall, this looks good to me. I ran the command and the output serves the purpose. The comment I left is a nit, since the actual error messages will be displayed anyway.

asfbot · 2017-01-13T22:34:34Z

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

asfbot · 2017-01-13T22:36:04Z

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

asfbot · 2017-01-13T22:36:22Z

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

ijuma

Thanks for the PR. I left a few comments.

ijuma · 2017-01-16T13:06:02Z

Nit: would it not be better to use \t instead of hardcoded spaces?

This one is still not using a tab.

oops, good catch!

ijuma · 2017-01-16T13:10:36Z

I think we should return a Scala collection like other methods (e.g. listGroups).

ijuma · 2017-01-16T13:16:49Z

We don't use the RPC terminology in Kafka, so it may confuse people to see this. It would be good to use terminology consistent with how the protocol is described in:

https://kafka.apache.org/protocol

Hmm. That document makes a lot of references to "API versions," so it seems like we can just say "API versions" here and drop the RPC.

ijuma · 2017-01-16T13:17:44Z

There is no "Consumer" being used here right?

Right, good catch.

ijuma · 2017-01-16T13:19:46Z

Interpolation calls toString by default, so the no-args versions are not needed.

Should we be prefixing v with a blurb stating that there was an error retrieving the data for that broker?

Ah... yeah, we can drop the .toString from the broker line.

I'll add an ERROR prefix to the Failure line.

ijuma · 2017-01-16T13:20:02Z

Nit: should be in the previous line.

ijuma · 2017-01-16T13:20:55Z

Nit: should be in the previous line and case is not needed here (if you're feeling generous, you could fix listAllGroups as well.

ijuma · 2017-01-16T13:23:23Z

You could replace the try/catch with Try(new NodeApiVersions(getApiVersions(broker))).

ijuma · 2017-01-16T13:25:35Z

Would it be better to name the shell script kafka-broker-api-versions and the command BrokerApiVersionsCommand? We could then drop the required list-api-versions option. I'm guessing you may have ideas of how to extend the command in the future, maybe you can elaborate on that a little?

I was thinking that eventually we might want to have a way to request API versions from only specific brokers. We may also want to have a way to pull the version string out of the requests they return. I agree that it's probably safe to drop the requested list-api-versions option. I would rather not add "api" to the name, since there may eventually be more to versioning, such as reading the version string sent over the wire, etc. Thoughts?

As discussed offline, we don't have a broker string at the moment. It's a bit hard to predict the future, but maybe this command should be a simple one focused on api versions. If we add a more general command later, we can change the shell script to invoke it with the right parameters.

fair enough. I will change it to kafka-broker-api-versions.sh

asfbot · 2017-01-17T20:49:21Z

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

asfbot · 2017-01-17T20:50:26Z

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

asfbot · 2017-01-17T21:23:20Z

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

ijuma

Just a few minor comments and I think we can merge this.

ijuma · 2017-01-18T09:39:48Z

As discussed offline, we don't have a broker string at the moment. It's a bit hard to predict the future, but maybe this command should be a simple one focused on api versions. If we add a more general command later, we can change the shell script to invoke it with the right parameters.

ijuma · 2017-01-18T09:40:29Z

This one is still not using a tab.

ijuma · 2017-01-18T09:40:52Z

II think this is unused.

asfbot · 2017-01-18T17:48:56Z

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

asfbot · 2017-01-18T17:49:18Z

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

asfbot · 2017-01-18T17:53:58Z

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

…ting versions from a broker

asfbot · 2017-01-18T23:31:33Z

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

asfbot · 2017-01-18T23:34:02Z

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

Author: Colin P. Mccabe <cmccabe@confluent.io> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>, Apurva Mehta <apurva.1618@gmail.com>, Ismael Juma <ismael@juma.me.uk> Closes #2184 from cmccabe/KAFKA-4457 (cherry picked from commit 4a6f2c6) Signed-off-by: Ismael Juma <ismael@juma.me.uk>

asfbot · 2017-01-19T00:09:53Z

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

Author: Colin P. Mccabe <cmccabe@confluent.io> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>, Apurva Mehta <apurva.1618@gmail.com>, Ismael Juma <ismael@juma.me.uk> Closes apache#2184 from cmccabe/KAFKA-4457

ewencp reviewed Dec 31, 2016

View reviewed changes

cmccabe force-pushed the KAFKA-4457 branch from f36cfd1 to 3a96ac1 Compare January 5, 2017 21:25

cmccabe force-pushed the KAFKA-4457 branch from 19280cd to b6aa9c9 Compare January 12, 2017 00:24

apurvam reviewed Jan 12, 2017

View reviewed changes

cmccabe force-pushed the KAFKA-4457 branch from b6aa9c9 to f92ad0e Compare January 12, 2017 22:50

apurvam reviewed Jan 13, 2017

View reviewed changes

apurvam approved these changes Jan 13, 2017

View reviewed changes

cmccabe force-pushed the KAFKA-4457 branch from f92ad0e to 954cf43 Compare January 13, 2017 21:46

ijuma reviewed Jan 16, 2017

View reviewed changes

ijuma reviewed Jan 18, 2017

View reviewed changes

cmccabe added 2 commits January 18, 2017 14:40

KAFKA-4457. Add BrokerVersionCommand

7719dfb

BrokerVersionCommand: print each version on a separate line

ad98a77

cmccabe added 4 commits January 18, 2017 14:40

BrokerVersionCommand: print the exception when there is a problem get…

a93798a

…ting versions from a broker

Whitespace and style fixes, plus add ERROR text for exceptions

a7ad9c1

Style fixes, and rename BrokerVersionCommand to BrokerApiVersionCommand

adee5dc

Add testing

55b7f6e

cmccabe force-pushed the KAFKA-4457 branch from 8a7be54 to 55b7f6e Compare January 18, 2017 22:43

asfgit closed this in 4a6f2c6 Jan 18, 2017

cmccabe deleted the KAFKA-4457 branch May 20, 2019 18:33

Conversation

cmccabe commented Nov 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 5, 2017

Uh oh!

asfbot commented Jan 5, 2017

Uh oh!

asfbot commented Jan 5, 2017

Uh oh!

asfbot commented Jan 12, 2017

Uh oh!

asfbot commented Jan 12, 2017

Uh oh!

asfbot commented Jan 12, 2017

Uh oh!

apurvam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 12, 2017

Uh oh!

asfbot commented Jan 12, 2017

Uh oh!

asfbot commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apurvam left a comment

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 13, 2017

Uh oh!

asfbot commented Jan 13, 2017

Uh oh!

asfbot commented Jan 13, 2017

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe Jan 17, 2017 • 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!

cmccabe Jan 17, 2017 •

edited

Loading

cmccabe Jan 17, 2017 •

edited

Loading