Re: [PR] KAFKA-19624: Improving consistency of command-line arguments for consumer performance tests [kafka]

["AndrewJSchofield (via GitHub)" <gi...@apache.org>]

AndrewJSchofield commented on code in PR #20385:
URL: https://github.com/apache/kafka/pull/20385#discussion_r2314318522


##########
tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java:
##########
@@ -291,26 +297,41 @@ public ConsumerPerfOptions(String[] args) {
                 .describedAs("size")
                 .ofType(Integer.class)
                 .defaultsTo(1024 * 1024);
+            commandPropertiesOpt = parser.accepts("command-property", "Kafka consumer related configuration properties like client.id. " +
+                    "These configs take precedence over those passed via --command-config or --consumer.config.")
+                .withRequiredArg()
+                .describedAs("prop1=val1,prop2=val2...")

Review Comment:
   I do not think the code handles the format as described. For example, if I use `--command-property client.id=c1,check.crcs=false`, then the client ID is `"c1,check.crcs=false"`. The tricky part is that `kafka-producer-perf-test` uses a different argument parsing library and it supports `--command-property client.id=c1 bootstrap.servers=localhost:9092` with a space used as a separator. That's very non-standard but it's already there for that tool at least.
   
   Personally, I think that the easiest is to support `prop=val` only and permit multiple instances of `--command-property`, so someone can do `--command-property client.id=c1 --command-property check.crcs=false`. I didn't think the details would be subtle. wdyt?
   
   The same comment applies to the share consumer perf test also.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org