tombentley commented on code in PR #11781:
URL: https://github.com/apache/kafka/pull/11781#discussion_r897830233
##########
connect/runtime/src/main/java/org/apache/kafka/connect/util/TopicAdmin.java:
##########
@@ -274,18 +276,23 @@ public static NewTopicBuilder defineTopic(String
topicName) {
* @param adminConfig the configuration for the {@link Admin}
*/
public TopicAdmin(Map<String, Object> adminConfig) {
- this(adminConfig, Admin.create(adminConfig));
+ this(adminConfig.get(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG),
Admin.create(adminConfig));
}
- // visible for testing
- TopicAdmin(Map<String, Object> adminConfig, Admin adminClient) {
- this(adminConfig, adminClient, true);
+ /**
+ * Create a new topic admin using the provided {@link Admin}
+ *
+ * @param bootstrapServers the Kafka cluster targeted by the admin
+ * @param adminClient the {@link Admin} to use under the hood
+ */
+ public TopicAdmin(Object bootstrapServers, Admin adminClient) {
Review Comment:
This constructor kinda confuses the ownership of the `Admin` client. I think
things are cleaner when the TopicAdmin instantiates (and thus owns the
`admin`). Note, it looks like there are no callers for `TopicAdmin.admin`. It
seems that the call sites in `doBuild` could simply pass the map of configs
(and the `bootstrapServers` looked up from that), rather than instantiating the
`admin` and then passing it to the TopicAdmin.
Obviously the test code has slightly different requirements, meaning we
still need this constructor. I did also wonder whether we could also get rid of
`bootstrapServers` by defining `toString` on `KafkaAdminClient` and using that
for the logging and exceptions here in `TopicAdmin`. Perhaps that's worth a
followup PR at some point, (though perhaps there are benefits to hiding
bootstrap servers from receivers of clients).
--
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
