C0urante commented on code in PR #14313:
URL: https://github.com/apache/kafka/pull/14313#discussion_r1318694288
##########
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:
I'm not a huge fan of unchecked type casts when I can avoid them, and the
default of `"<unknown>"` that we use in the root constructor should alleviate
any concerns about NPEs. I guess we could maybe do something like
`adminConfig.getOrDefault(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG,
"<unknown>").toString()` (though this would be susceptible to NPEs if
`adminConfig` contained a null value for that key), but I agree that it's not a
blocker.
Happy to review if you want to submit that follow-up patch, though!
--
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
