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
