Copilot commented on code in PR #21080:
URL: https://github.com/apache/kafka/pull/21080#discussion_r3636336228
##########
clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java:
##########
@@ -1630,4 +1654,110 @@ public KafkaException getAndClearFailure() {
return failure;
}
}
+
+ private void bootstrapMetadataWithNodes(Metadata metadata, List<Node>
nodes) {
+ List<InetSocketAddress> serverAddresses = new ArrayList<>();
+ nodes.forEach(node -> serverAddresses.add(new
InetSocketAddress(node.host(), node.port())));
+ metadata.bootstrap(serverAddresses);
+ }
+
+ private void bootstrapMetadata(Metadata metadata) {
+ List<InetSocketAddress> serverAddresses = new ArrayList<>(List.of(
+ new InetSocketAddress("localhost0", 8000),
+ new InetSocketAddress("localhost1", 8000)
+ ));
+ metadata.bootstrap(serverAddresses);
Review Comment:
These helper methods use new InetSocketAddress("localhost0", ...) /
("localhost1", ...) which can trigger real DNS resolution during test
execution. Using createUnresolved avoids environmental DNS dependencies and
reduces flakiness.
This issue also appears on line 1673 of the same file.
##########
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:
The Javadoc for handleEmptyNodeList() says it will "return null" when
bootstrap is enabled but not yet complete, but the method actually returns a
non-null LeastLoadedNode with a null node. This mismatch is confusing for
callers/readers; update the comment to describe the real behavior.
##########
clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java:
##########
@@ -60,6 +60,12 @@ public class CommonClientConfigs {
+ "resolve each
bootstrap address into a list of canonical names. After "
+ "the bootstrap phase,
this behaves the same as <code>use_all_dns_ips</code>.";
+ public static final String BOOTSTRAP_RESOLVE_TIMEOUT_MS_CONFIG =
"bootstrap.resolve.timeout.ms";
+ public static final long DEFAULT_BOOTSTRAP_RESOLVE_TIMEOUT_MS = 2 * 60 *
1000L;
+ public static final String BOOTSTRAP_RESOLVE_TIMEOUT_MS_DOC = "Maximum
amount of time clients can spend trying to" +
+ " resolve for the bootstrap server address. If the resolution cannot
be completed within this timeframe, a " +
+ "<code>BootstrapResolutionException</code> will be thrown.";
Review Comment:
The documentation string for bootstrap.resolve.timeout.ms has an awkward
phrase ("resolve for the bootstrap server address"). Rewording it improves
clarity.
--
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]
