KAFKA-4307 Inconsistent parameters between console producer and consumer #2161

baluchicken · 2016-11-23T08:48:39Z

@gwenshap please review

ijuma · 2016-11-24T12:46:16Z

Thanks for the PR. A few comments:

We need to maintain compatibility, so the older parameter should be supported but deprecated.
It would be great to do the same change for any other class that uses --broker-list (VerifiableConsumer, VerifiableProducer, etc.)
It would be good to update the documentation and system tests to use bootstrap-server.

baluchicken · 2016-11-28T13:26:04Z

Thanks for the review, I will start to work on the suggested modifications. I will update this pull request soon.

baluchicken · 2016-12-02T15:54:15Z

@ijuma I updated the pull request, with the suggested modifications. I forgot to update the documentation but I will do that maybe this weekend or in the beginning of the next one. Also there are couple of files where I don't know how to proceed.
These files are: GetOffsetshell, ReplicaVerificationTool, SimpleConsumerShell, Consumer Performance, VerifiableLog4jAppender. One of those are refactored to use broker-list instead of zookeeper, others are still using the old implementation as far as I can tell because the metadata.broker.list property is in use.

baluchicken · 2016-12-08T08:39:08Z

Updated the pull request with the documentation, and I also fixed the stylecheck errors.

ijuma

Thanks for the PR. Left a couple of comments. Also, it would be good to rebase the branch.

ijuma · 2016-12-21T15:13:37Z

core/src/main/scala/kafka/tools/ConsoleProducer.scala

      .withRequiredArg
      .describedAs("broker-list")
      .ofType(classOf[String])
+    val bootstrapServerListOpt = parser.accepts("bootstrap-server", "REQUIRED (unless old producer is used): The bootstrap server list string in the form HOST1:PORT1,HOST2:PORT2.")


I don't understand this. bootstrap-server and broker-list should be equivalent. The user should provide exactly one of the two. And it can be used with both old and new producers. bootstrap-server is recommended going forward for consistency with the console consumer. I think you did it right for the VerifiableConsumer, for example.

Also, we can call the val simply bootstrapServerOpt.

ijuma · 2016-12-21T15:17:30Z

tools/src/main/java/org/apache/kafka/tools/VerifiableProducer.java

+                .action(store())
+                .type(String.class)
+                .metavar("HOST1:PORT1[,HOST2:PORT2[...]]")
+                .dest("bootstrapServerList")


I'd call this just bootstrapServer. Similarly elsewhere.

baluchicken · 2017-01-05T13:25:25Z

@ijuma Updated the PR with the suggested changes.

asfbot · 2017-01-05T14:05:02Z

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

asfbot · 2017-01-05T14:05:13Z

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

asfbot · 2017-01-05T14:06:17Z

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

ijuma · 2017-01-06T15:00:32Z

core/src/main/scala/kafka/tools/ConsoleProducer.scala


    val useOldProducer = options.has(useOldProducerOpt)
-    val topic = options.valueOf(topicOpt)
+    val bootstrapServer = options.valueOf(bootstrapServerOpt)


The general pattern over all files should be that bootstrapServer has the value of bootstrapServerOpt or brokerListOpt. Since we validate that only one of them can be set, this is OK. If we do it that way, we don't have to check two fields everywhere (or worse forget to check one of them like it seems to be happening here in getNewProducerProps).

Ohh sorry, yeah you are right. I have one question, why can the user use the old-producer with the bootstrap-server? The broker-list and the bootstrap-server uses different properties "metadata.broker.list" vs "bootstrap.servers".

It's to make it easier for users. They don't have to switch the CLI param to switch between new/old producer. After all broker-list is not the same as metadata.broker.list either.

baluchicken · 2017-01-11T15:17:06Z

@ijuma thanks for the review. Updated the pull request as you suggested.

asfbot · 2017-01-11T15:55:48Z

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

asfbot · 2017-01-11T15:57:47Z

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

asfbot · 2017-01-11T16:50:58Z

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

asfbot · 2017-01-12T10:49:20Z

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

asfbot · 2017-01-12T10:52:22Z

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

asfbot · 2017-01-12T12:11:32Z

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

ijuma

Thanks for the update, left one comment.

ijuma · 2017-01-23T13:26:55Z

core/src/main/scala/kafka/tools/ConsoleProducer.scala


    val useOldProducer = options.has(useOldProducerOpt)
+    val bootstrapServer = List(bootstrapServerOpt, brokerListOpt).flatMap(x => Option(options.valueOf(x))).head


This should be done after we validate arguments. Otherwise, we can get a NoSuchElementException if neither bootstrap-server or broker-list is passed by the user. Same for other classes. Worth adding a test for this.

baluchicken · 2017-01-23T19:01:21Z

@ijuma thanks for the review, updated the files.

asfbot · 2017-01-23T19:40:55Z

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

asfbot · 2017-01-23T19:43:15Z

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

asfbot · 2017-01-23T19:49:58Z

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

ijuma · 2017-01-27T11:46:10Z

@ewencp, do you think it's OK to include this in 0.10.2? It technically adds a command-line parameters to some tools, but that parameter already exists in a number of tools, it's just making them consistent.

ewencp · 2017-01-27T23:10:29Z

@ijuma I think it's fine as long as we're confident we didn't break compatibility anywhere.

baluchicken · 2017-02-03T11:30:44Z

Resolved the conflicts, made some modifications on testBrokerListAndBootstrapServerOptionMissing

asfbot · 2017-04-12T10:32:45Z

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

asfbot · 2017-04-12T10:36:05Z

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

asfbot · 2017-04-12T11:10:44Z

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

baluchicken · 2017-06-02T07:24:57Z

@gwenshap if you have time can you please take a look? Thanks.

asfbot · 2017-06-02T08:25:10Z

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

asfbot · 2017-06-02T08:34:31Z

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

ijuma · 2017-08-04T13:20:26Z

Sorry for the delay. Can you please resolve the conflicts so that I can hopefully take a final look and we can finally merge this.

baluchicken · 2017-08-04T13:50:15Z

No worries:). I resolved the conflicts and extended the stream docs.

asfgit · 2017-08-04T15:09:25Z

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

baluchicken · 2017-08-08T08:23:52Z

Resolved the conflicts. @ijuma if you have time can you please review?

asfgit · 2017-08-08T09:22:50Z

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

asfgit · 2017-08-08T09:40:09Z

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

ijuma · 2017-08-08T22:28:45Z

Yes, I started looking into this. I think there may be scope for some sharing of code. I'll share a sketch tomorrow.

asfgit · 2017-08-15T14:16:30Z

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

asfgit · 2017-08-15T14:40:57Z

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

…st is present

mimaison · 2022-02-10T16:26:40Z

I believe this has been addressed by KIP-499, so closing this PR.

baluchicken force-pushed the KAFKA-4307 branch from f2b2775 to 0283187 Compare December 2, 2016 15:39

baluchicken force-pushed the KAFKA-4307 branch from 0283187 to c961062 Compare December 4, 2016 20:12

ijuma reviewed Dec 21, 2016

View reviewed changes

baluchicken force-pushed the KAFKA-4307 branch from c961062 to cf9da39 Compare January 5, 2017 13:24

ijuma reviewed Jan 6, 2017

View reviewed changes

baluchicken force-pushed the KAFKA-4307 branch from cf9da39 to ffdfa9d Compare January 11, 2017 15:15

ijuma reviewed Jan 23, 2017

View reviewed changes

baluchicken force-pushed the KAFKA-4307 branch from c22c73f to 63310b9 Compare February 3, 2017 11:29

baluchicken force-pushed the KAFKA-4307 branch from 8737683 to ad3e25a Compare August 4, 2017 13:48

baluchicken force-pushed the KAFKA-4307 branch from ad3e25a to 0ed3d82 Compare August 8, 2017 08:23

baluchicken force-pushed the KAFKA-4307 branch from 0ed3d82 to 9022239 Compare August 15, 2017 13:15

baluchicken added 9 commits August 21, 2017 16:58

KAFKA-4307 Inconsistent parameters between console producer and consumer

ad8edbb

Update documentation

eb8c044

Rename BootstrapServerList/Opt to BootstrapServer/Opt

533bc6c

Make bootstrap-server and broker-list equivalent

11555e3

Address problems mentioned in review

5ccb57f

Remove the duplicated ToolsUtils.validatePortorDie

e456e27

Avoid NoSuchElementException if neither bootstrap-server or broker-li…

3f307d3

…st is present

Rebase modifications to work on trunk

983fa51

Change --broker-list to --bootstrap-server in stream docs

e2bbd04

baluchicken force-pushed the KAFKA-4307 branch from 9022239 to e2bbd04 Compare August 21, 2017 15:00

mimaison closed this Feb 10, 2022


		val useOldProducer = options.has(useOldProducerOpt)
		val bootstrapServer = List(bootstrapServerOpt, brokerListOpt).flatMap(x => Option(options.valueOf(x))).head

KAFKA-4307 Inconsistent parameters between console producer and consumer #2161

KAFKA-4307 Inconsistent parameters between console producer and consumer #2161

Conversation

baluchicken commented Nov 23, 2016

ijuma commented Nov 24, 2016

baluchicken commented Nov 28, 2016

baluchicken commented Dec 2, 2016

baluchicken commented Dec 8, 2016

ijuma left a comment

Choose a reason for hiding this comment

ijuma Dec 21, 2016

Choose a reason for hiding this comment

ijuma Dec 21, 2016

Choose a reason for hiding this comment

baluchicken commented Jan 5, 2017

asfbot commented Jan 5, 2017

asfbot commented Jan 5, 2017

asfbot commented Jan 5, 2017

ijuma Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

baluchicken Jan 9, 2017

Choose a reason for hiding this comment

ijuma Jan 9, 2017

Choose a reason for hiding this comment

baluchicken commented Jan 11, 2017

asfbot commented Jan 11, 2017

asfbot commented Jan 11, 2017

asfbot commented Jan 11, 2017

asfbot commented Jan 12, 2017

asfbot commented Jan 12, 2017

asfbot commented Jan 12, 2017

ijuma left a comment

Choose a reason for hiding this comment

ijuma Jan 23, 2017

Choose a reason for hiding this comment

baluchicken commented Jan 23, 2017

asfbot commented Jan 23, 2017

asfbot commented Jan 23, 2017

asfbot commented Jan 23, 2017

ijuma commented Jan 27, 2017

ewencp commented Jan 27, 2017

baluchicken commented Feb 3, 2017

asfbot commented Apr 12, 2017

asfbot commented Apr 12, 2017

asfbot commented Apr 12, 2017

baluchicken commented Jun 2, 2017

asfbot commented Jun 2, 2017

asfbot commented Jun 2, 2017

ijuma commented Aug 4, 2017

baluchicken commented Aug 4, 2017

asfgit commented Aug 4, 2017

baluchicken commented Aug 8, 2017

asfgit commented Aug 8, 2017

asfgit commented Aug 8, 2017

ijuma commented Aug 8, 2017

asfgit commented Aug 15, 2017

asfgit commented Aug 15, 2017

mimaison commented Feb 10, 2022

ijuma Jan 6, 2017 •

edited

Loading