Copilot commented on code in PR #7745:
URL: https://github.com/apache/ignite-3/pull/7745#discussion_r2911697469
##########
modules/client/src/test/java/org/apache/ignite/client/ClientDnsDiscoveryTest.java:
##########
@@ -37,7 +37,9 @@
import org.apache.ignite.internal.client.TcpIgniteClient;
import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
import org.apache.ignite.internal.testframework.IgniteTestUtils;
+import org.apache.ignite.lang.LoggerFactory;
import org.apache.ignite.network.ClusterNode;
+import org.jspecify.annotations.Nullable;
Review Comment:
`@Nullable` is imported from `org.jspecify.annotations.Nullable`, but the
rest of the client tests in this module consistently use
`org.jetbrains.annotations.Nullable`. Please switch to the JetBrains annotation
here to keep nullability annotations consistent across tests (and avoid mixing
frameworks in a single module).
##########
modules/client/src/test/java/org/apache/ignite/client/ClientDnsDiscoveryTest.java:
##########
@@ -234,10 +236,48 @@ void testMultipleIpsSameNode() throws
InterruptedException {
}
}
+ @Test
+ void testMultipleEndpointsSameNodeLogsWarning() throws
InterruptedException {
+ // Two distinct IPs that both resolve to the same node (server3).
+ AtomicReference<String[]> resolvedAddressesRef = new
AtomicReference<>(new String[]{loopbackAddress, hostAddress});
+ TestLoggerFactory loggerFactory = new TestLoggerFactory("test-client");
+ String[] addresses = {"my-cluster:" + server3.port()};
+
+ var cfg = getClientConfiguration(addresses, 0L, resolvedAddressesRef,
loggerFactory);
+
+ List<?> channelHolders;
+
+ try (var client = TcpIgniteClient.startAsync(cfg).join()) {
+ assertDoesNotThrow(() -> client.tables().tables());
+ loggerFactory.waitForLogMatches(".*Multiple distinct endpoints
resolve to the same server node.*", 3000);
+
Review Comment:
The log wait timeout (3000 ms) is shorter than similar logging tests in this
module and may cause flakes on slow CI. Consider increasing it (e.g., to 5000
ms, as used in other `TestLoggerFactory`-based tests) to make the test more
stable.
##########
modules/client/src/test/java/org/apache/ignite/client/ClientDnsDiscoveryTest.java:
##########
@@ -234,10 +236,48 @@ void testMultipleIpsSameNode() throws
InterruptedException {
}
}
+ @Test
+ void testMultipleEndpointsSameNodeLogsWarning() throws
InterruptedException {
+ // Two distinct IPs that both resolve to the same node (server3).
+ AtomicReference<String[]> resolvedAddressesRef = new
AtomicReference<>(new String[]{loopbackAddress, hostAddress});
+ TestLoggerFactory loggerFactory = new TestLoggerFactory("test-client");
+ String[] addresses = {"my-cluster:" + server3.port()};
+
+ var cfg = getClientConfiguration(addresses, 0L, resolvedAddressesRef,
loggerFactory);
+
+ List<?> channelHolders;
+
+ try (var client = TcpIgniteClient.startAsync(cfg).join()) {
+ assertDoesNotThrow(() -> client.tables().tables());
+ loggerFactory.waitForLogMatches(".*Multiple distinct endpoints
resolve to the same server node.*", 3000);
+
+ List<ClusterNode> connections = client.connections();
+ assertEquals(2, connections.size());
+ assertEquals("server3", connections.get(0).name());
+ assertEquals("server3", connections.get(1).name());
+
+ channelHolders = IgniteTestUtils.getFieldValue(((TcpIgniteClient)
client).channel(), "channels");
+ assertEquals(2, channelHolders.size());
+ }
+
+ for (Object holder : channelHolders) {
+ boolean isClosed = IgniteTestUtils.getFieldValue(holder, "close");
+ assertTrue(isClosed, "Channel holder should be closed after client
close");
+ }
+ }
+
private static IgniteClientConfigurationImpl getClientConfiguration(
String[] addresses,
long backgroundReResolveAddressesInterval,
AtomicReference<String[]> resolvedAddressesRef
+ ) {
+ return getClientConfiguration(addresses,
backgroundReResolveAddressesInterval, resolvedAddressesRef, null);
+ }
+ private static IgniteClientConfigurationImpl getClientConfiguration(
+ String[] addresses,
Review Comment:
Add a blank line between these overloaded `getClientConfiguration` methods
to match the spacing used between other methods in this test class and improve
readability.
##########
modules/client/src/main/java/org/apache/ignite/internal/client/ReliableChannel.java:
##########
@@ -961,6 +967,18 @@ private CompletableFuture<ClientChannel>
getOrCreateChannelAsync() {
ClusterNode newNode = ch.protocolContext().clusterNode();
+ // Check if another endpoint already connected to this
node.
+ ClientChannelHolder existingHolder =
nodeChannelsByName.get(newNode.name());
+ if (existingHolder != null && existingHolder != this) {
+ log.warn("Multiple distinct endpoints resolve to the
same server node [nodeName={}, nodeId={}, "
+ + "existingEndpoint={}, newEndpoint={}]. This
represents a misconfiguration. "
+ + "Both connections will remain active to
avoid disrupting ongoing operations.",
+ newNode.name(),
+ newNode.id(),
+ existingHolder.chCfg.getAddress(),
+ chCfg.getAddress());
+ }
Review Comment:
This warning will be emitted every time one of the two endpoints reconnects
while the other is active (the map entry flips between holders), which can
produce repetitive warnings under intermittent network conditions. Consider
de-duplicating/rate-limiting the warning (e.g., log once per node name, or only
when the condition is first detected) to avoid noisy logs.
--
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]
