[GitHub] [kafka] justinrlee commented on a diff in pull request #12797: MINOR: Remove requirement to specify --bootstrap-server in utility scripts if specified in property file

[GitBox <gi...@apache.org>]

justinrlee commented on code in PR #12797:
URL: https://github.com/apache/kafka/pull/12797#discussion_r1042238071


##########
core/src/main/scala/kafka/admin/BrokerApiVersionsCommand.scala:
##########
@@ -95,8 +96,6 @@ object BrokerApiVersionsCommand {
 
     def checkArgs(): Unit = {
       CommandLineUtils.printHelpAndExitIfNeeded(this, "This tool helps to retrieve broker version information.")
-      // check required args
-      CommandLineUtils.checkRequiredArgs(parser, options, bootstrapServerOpt)

Review Comment:
   Hey @dengziming thanks for the comment. This is sort of addressed in the PR comment. Basically, if the user doesn't pass bootstrap server in either arg or property file, you get an exit code of 1 and fairly intuitive error `Missing required configuration "bootstrap.servers" which has no default value.`.
   
   I was thinking this would be sufficient, but I can look at adding explicit error handling if you think that's better. Was trying to keep this a relatively small PR.
   
   > For example, specifying ONLY the property file without bootstrap.servers specified will result in output that looks like this, which is fairly intuitive to address:
   
   ```
   ./kafka_2.13-3.4.0-SNAPSHOT/bin/kafka-broker-api-versions.sh --command-config client-no-bootstrap.properties
   Exception in thread "main" org.apache.kafka.common.config.ConfigException: Missing required configuration "bootstrap.servers" which has no default value.
   	at org.apache.kafka.common.config.ConfigDef.parseValue(ConfigDef.java:493)
   	at org.apache.kafka.common.config.ConfigDef.parse(ConfigDef.java:483)
   	at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:113)
   	at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:146)
   	at kafka.admin.BrokerApiVersionsCommand$AdminClient$AdminConfig.<init>(BrokerApiVersionsCommand.scala:265)
   	at kafka.admin.BrokerApiVersionsCommand$AdminClient$.create(BrokerApiVersionsCommand.scala:269)
   	at kafka.admin.BrokerApiVersionsCommand$AdminClient$.create(BrokerApiVersionsCommand.scala:267)
   	at kafka.admin.BrokerApiVersionsCommand$.createAdminClient(BrokerApiVersionsCommand.scala:79)
   	at kafka.admin.BrokerApiVersionsCommand$.execute(BrokerApiVersionsCommand.scala:60)
   	at kafka.admin.BrokerApiVersionsCommand$.main(BrokerApiVersionsCommand.scala:55)
   	at kafka.admin.BrokerApiVersionsCommand.main(BrokerApiVersionsCommand.scala)
   ```



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