Copilot commented on code in PR #7848:
URL: https://github.com/apache/ignite-3/pull/7848#discussion_r2974814053
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientFailoverSocket.cs:
##########
@@ -778,10 +778,21 @@ private async ValueTask<ClientSocket>
ConnectAsync(SocketEndpoint endpoint)
$"Cluster ID mismatch: expected={_clusterId},
actual={socket.ConnectionContext.ClusterIds.StringJoin()}");
}
+ // Check if another endpoint already connected to this node.
+ var nodeName = socket.ConnectionContext.ClusterNode.Name;
+ if (_endpointsByName.TryGetValue(nodeName, out var
existingEndpoint) && existingEndpoint != endpoint)
Review Comment:
The warning is intended for *distinct endpoints*, but the condition uses
reference inequality (`existingEndpoint != endpoint`). If a reconnect path
creates a new `SocketEndpoint` instance for the same underlying `EndPoint`,
this will log a false-positive. Compare `existingEndpoint.EndPoint` to
`endpoint.EndPoint` (or use an explicit comparer) so the log only triggers when
the resolved endpoint address actually differs.
```suggestion
if (_endpointsByName.TryGetValue(nodeName, out var
existingEndpoint) &&
!Equals(existingEndpoint.EndPoint, endpoint.EndPoint))
```
##########
modules/platforms/dotnet/Apache.Ignite.Tests/IgniteServerBase.cs:
##########
@@ -20,12 +20,14 @@ namespace Apache.Ignite.Tests;
using System;
using System.Collections.Concurrent;
using System.Diagnostics.CodeAnalysis;
+using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Threading;
using System.Threading.Tasks;
using Internal.Buffers;
using NUnit.Framework;
+using NUnit.Framework.Internal;
Review Comment:
This introduces a dependency on `NUnit.Framework.Internal` / `TestUtils`,
which is not part of NUnit’s public API and can change without notice. Prefer
using NUnit’s public polling assertions (e.g., `Assert.That(...,
Is.EqualTo(...).After(...))`) or a small local wait helper to avoid relying on
internal NUnit APIs.
```suggestion
```
##########
modules/platforms/dotnet/Apache.Ignite/Internal/LogMessages.cs:
##########
@@ -279,4 +279,13 @@ internal static partial void LogTxStartedTrace(
Level = LogLevel.Trace,
EventId = 1038)]
internal static partial void LogEndpointListUpdatedTrace(this ILogger
logger, string added, string removed);
+
+ [LoggerMessage(
+ Message = "Multiple distinct endpoints resolve to the same server node
[nodeName={NodeName}, nodeId={NodeId}, " +
+ "existingEndpoint={ExistingEndpoint},
newEndpoint={NewEndpoint}]. This represents a misconfiguration. " +
+ "Both connections will remain active to avoid disrupting
ongoing operations.",
Review Comment:
The message states that both connections will remain active, but in
`ConnectAsync` the `_endpointsByName[nodeName]` mapping is overwritten with the
new endpoint. If this mapping is used for routing/selection, the behavior is
effectively “last endpoint wins” even if both sockets remain open. Consider
clarifying the message (e.g., that an additional connection is kept open but
the preferred mapping may change), or adjust the logic to keep the existing
mapping stable when a duplicate endpoint is detected.
```suggestion
"Both connections will remain active to avoid disrupting
ongoing operations, but the new endpoint will " +
"be preferred for subsequent routing/selection.",
```
##########
modules/platforms/dotnet/Apache.Ignite.Tests/IgniteServerBase.cs:
##########
@@ -79,6 +83,8 @@ public void DropExistingConnection()
}
}
+ public void WaitForConnections(int count) => TestUtils.WaitForCondition(()
=> ConnectionCount == count, 5000);
Review Comment:
This introduces a dependency on `NUnit.Framework.Internal` / `TestUtils`,
which is not part of NUnit’s public API and can change without notice. Prefer
using NUnit’s public polling assertions (e.g., `Assert.That(...,
Is.EqualTo(...).After(...))`) or a small local wait helper to avoid relying on
internal NUnit APIs.
##########
modules/platforms/dotnet/Apache.Ignite.Tests/ClientFailoverSocketTests.cs:
##########
@@ -51,4 +53,40 @@ public async Task
TestBackgroundConnectionProcessDoesNotBlockOperations()
Assert.AreEqual(0, tables.Count);
Assert.AreEqual(1, client.GetConnections().Count);
}
+
+ [Test]
+ public async Task TestMultipleEndpointsSameNodeLogsWarning()
+ {
+ ClientFailoverSocket.ResetGlobalEndpointIndex();
+
+ using var server = new FakeServer(nodeName: "test-node")
+ {
+ AllowMultipleConnections = true
+ };
+
+ var logger = new ListLoggerFactory([LogLevel.Warning]);
+
+ var clientCfg = new IgniteClientConfiguration
+ {
+ Endpoints = { $"127.0.0.1:{server.Port}",
$"localhost:{server.Port}" },
+ LoggerFactory = logger,
+ ReconnectInterval = TimeSpan.Zero
+ };
+
+ using var client = await IgniteClient.StartAsync(clientCfg);
Review Comment:
The test disposes `client` manually while it is also declared as `using
var`, which will dispose it a second time at scope exit. If `Dispose` is not
strictly idempotent, this can make the test flaky. Consider removing `using
var` and disposing explicitly (ideally via `try/finally`), or restructure into
a nested scope so the additional assertions happen after the `using` block ends.
```suggestion
var client = await IgniteClient.StartAsync(clientCfg);
```
##########
modules/platforms/dotnet/Apache.Ignite.Tests/ClientFailoverSocketTests.cs:
##########
@@ -51,4 +53,40 @@ public async Task
TestBackgroundConnectionProcessDoesNotBlockOperations()
Assert.AreEqual(0, tables.Count);
Assert.AreEqual(1, client.GetConnections().Count);
}
+
+ [Test]
+ public async Task TestMultipleEndpointsSameNodeLogsWarning()
+ {
+ ClientFailoverSocket.ResetGlobalEndpointIndex();
+
+ using var server = new FakeServer(nodeName: "test-node")
+ {
+ AllowMultipleConnections = true
+ };
+
+ var logger = new ListLoggerFactory([LogLevel.Warning]);
+
+ var clientCfg = new IgniteClientConfiguration
+ {
+ Endpoints = { $"127.0.0.1:{server.Port}",
$"localhost:{server.Port}" },
+ LoggerFactory = logger,
+ ReconnectInterval = TimeSpan.Zero
+ };
+
+ using var client = await IgniteClient.StartAsync(clientCfg);
+
+ client.WaitForConnections(2);
+ server.WaitForConnections(2);
+
+ var log = logger.GetLogString();
+ StringAssert.Contains("Multiple distinct endpoints resolve to the same
server node", log);
+ StringAssert.Contains("test-node", log);
+
+ // ReSharper disable once DisposeOnUsingVariable
+ client.Dispose();
+
+ // Ensure that duplicate connections are cleaned up properly.
+ client.WaitForConnections(0);
+ server.WaitForConnections(0);
Review Comment:
The test disposes `client` manually while it is also declared as `using
var`, which will dispose it a second time at scope exit. If `Dispose` is not
strictly idempotent, this can make the test flaky. Consider removing `using
var` and disposing explicitly (ideally via `try/finally`), or restructure into
a nested scope so the additional assertions happen after the `using` block ends.
```suggestion
var client = await IgniteClient.StartAsync(clientCfg);
var clientDisposed = false;
try
{
client.WaitForConnections(2);
server.WaitForConnections(2);
var log = logger.GetLogString();
StringAssert.Contains("Multiple distinct endpoints resolve to
the same server node", log);
StringAssert.Contains("test-node", log);
// ReSharper disable once DisposeOnUsingVariable
client.Dispose();
clientDisposed = true;
// Ensure that duplicate connections are cleaned up properly.
client.WaitForConnections(0);
server.WaitForConnections(0);
}
finally
{
if (!clientDisposed)
{
client.Dispose();
}
}
```
--
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]
