philipnee commented on code in PR #14313: URL: https://github.com/apache/kafka/pull/14313#discussion_r1316332331
########## connect/runtime/src/main/java/org/apache/kafka/connect/util/TopicAdmin.java: ########## @@ -286,11 +286,16 @@ public static NewTopicBuilder defineTopic(String topicName) { * @param adminConfig the configuration for the {@link Admin} */ public TopicAdmin(Map<String, Object> adminConfig) { - this(adminConfig.get(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG), Admin.create(adminConfig)); + this(adminConfig, Admin.create(adminConfig)); } - public TopicAdmin(Object bootstrapServers, Admin adminClient) { - this(bootstrapServers, adminClient, true); + public TopicAdmin(Map<String, Object> adminConfig, Admin adminClient) { + this(bootstrapServers(adminConfig), adminClient, true); + } + + // visible for testing + TopicAdmin(Admin adminClient) { + this(null, adminClient, true); Review Comment: sorry for being a type police 😆 : But this logic annoys me ``` public TopicAdmin(Object bootstrapServers, Admin adminClient) { this(bootstrapServers, adminClient, true); } // visible for testing TopicAdmin(Object bootstrapServers, Admin adminClient, boolean logCreation) { this.admin = adminClient; this.bootstrapServers = bootstrapServers != null ? bootstrapServers.toString() : "<unknown>"; this.logCreation = logCreation; } ``` Can we just do map.getOrDefault() and perform an inline type cast? In general passing nulls around confuses me... ########## connect/runtime/src/main/java/org/apache/kafka/connect/util/TopicAdmin.java: ########## @@ -286,11 +286,16 @@ public static NewTopicBuilder defineTopic(String topicName) { * @param adminConfig the configuration for the {@link Admin} */ public TopicAdmin(Map<String, Object> adminConfig) { - this(adminConfig.get(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG), Admin.create(adminConfig)); + this(adminConfig, Admin.create(adminConfig)); } - public TopicAdmin(Object bootstrapServers, Admin adminClient) { - this(bootstrapServers, adminClient, true); + public TopicAdmin(Map<String, Object> adminConfig, Admin adminClient) { + this(bootstrapServers(adminConfig), adminClient, true); + } + + // visible for testing + TopicAdmin(Admin adminClient) { + this(null, adminClient, true); Review Comment: also understood this is not your code ... So I can submit a minor patch for this if you don't want to modify your PR. ########## connect/runtime/src/main/java/org/apache/kafka/connect/util/TopicAdmin.java: ########## @@ -286,11 +286,16 @@ public static NewTopicBuilder defineTopic(String topicName) { * @param adminConfig the configuration for the {@link Admin} */ public TopicAdmin(Map<String, Object> adminConfig) { - this(adminConfig.get(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG), Admin.create(adminConfig)); + this(adminConfig, Admin.create(adminConfig)); } - public TopicAdmin(Object bootstrapServers, Admin adminClient) { - this(bootstrapServers, adminClient, true); + public TopicAdmin(Map<String, Object> adminConfig, Admin adminClient) { + this(bootstrapServers(adminConfig), adminClient, true); + } + + // visible for testing + TopicAdmin(Admin adminClient) { + this(null, adminClient, true); Review Comment: sorry for being a type police 😆 : But this logic annoys me ``` public TopicAdmin(Object bootstrapServers, Admin adminClient) { this(bootstrapServers, adminClient, true); } // visible for testing TopicAdmin(Object bootstrapServers, Admin adminClient, boolean logCreation) { this.admin = adminClient; this.bootstrapServers = bootstrapServers != null ? bootstrapServers.toString() : "<unknown>"; this.logCreation = logCreation; } ``` Can we just do map.getOrDefault() and perform an inline type cast? In general passing nulls around confuses me... -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org