chia7712 commented on code in PR #21080:
URL: https://github.com/apache/kafka/pull/21080#discussion_r3655161602
##########
clients/src/main/java/org/apache/kafka/clients/NetworkClient.java:
##########
@@ -835,6 +899,21 @@ public LeastLoadedNode leastLoadedNode(long now) {
}
}
+ /**
+ * Handle the case when there are no nodes available.
+ * <p>
+ * If bootstrap is disabled or already complete, throw
IllegalStateException.
+ * If bootstrap is enabled but not yet complete, return null to allow DNS
resolution to continue.
Review Comment:
this statement is out of date.
##########
clients/src/main/java/org/apache/kafka/clients/ClientUtils.java:
##########
@@ -201,6 +247,17 @@ public static NetworkClient
createNetworkClient(AbstractConfig config,
metricsGroupPrefix,
channelBuilder,
logContext);
+ ClientDnsLookup dnsLookup =
ClientDnsLookup.forConfig(config.getString(CommonClientConfigs.CLIENT_DNS_LOOKUP_CONFIG));
+
+ NetworkClient.BootstrapConfiguration bootstrapConfiguration =
+ (bootstrapServers != null && !bootstrapServers.isEmpty())
Review Comment:
Why do we have a path that does not require the bootstrap servers?
##########
clients/src/main/java/org/apache/kafka/clients/admin/internals/AdminMetadataManager.java:
##########
@@ -148,6 +149,17 @@ public void rebootstrap(long now) {
AdminMetadataManager.this.rebootstrap(now);
}
+ @Override
+ public boolean isBootstrapped() {
+ return bootstrapCluster != null;
+ }
+
+ @Override
+ public void bootstrap(List<InetSocketAddress> addresses) {
+ // Called by NetworkClient.ensureBootstrapped() after DNS
resolution succeeds
+ AdminMetadataManager.this.update(Cluster.bootstrap(addresses), 0);
Review Comment:
Would you mind adding comments for the magic `0`?
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -538,12 +575,19 @@ static KafkaAdminClient createInternal(
try {
// Since we only request node information, it's safe to pass true
for allowAutoTopicCreation (and it
// simplifies communication with older brokers)
- AdminBootstrapAddresses adminAddresses =
AdminBootstrapAddresses.fromConfig(config);
Review Comment:
It seems `AdminBootstrapAddresses` is useless. Could you remove it as well?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
