You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ptupitsyn (via GitHub)" <gi...@apache.org> on 2025/12/17 12:10:55 UTC
[PR] IGNITE-26892 .NET: improve DNS resolution logic [ignite-3]
ptupitsyn opened a new pull request, #7261:
URL: https://github.com/apache/ignite-3/pull/7261
(no comment)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26894 .NET: Improve DNS resolution logic [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn merged PR #7261:
URL: https://github.com/apache/ignite-3/pull/7261
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26892 .NET: Improve DNS resolution logic [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7261:
URL: https://github.com/apache/ignite-3/pull/7261#discussion_r2627093201
##########
modules/platforms/dotnet/Apache.Ignite/IgniteClientConfiguration.cs:
##########
@@ -179,10 +185,22 @@ public IgniteClientConfiguration(IgniteClientConfiguration other)
[DefaultValue("00:00:30")]
public TimeSpan ReconnectInterval { get; set; } = DefaultReconnectInterval;
+ /// <summary>
+ /// Gets or sets how long the resolved addresses will be considered valid.
+ /// <para />
+ /// Default is <see cref="DefaultReResolveAddressesInterval"/>.
+ /// Set to <see cref="TimeSpan.Zero"/> to disable periodic DNS re-resolve.
+ /// <para />
+ /// Ignite client resolves the provided hostnames into multiple IP addresses, each corresponds to a cluster node.
Review Comment:
The documentation says "each corresponds to a cluster node", but in the Java version (line 233 of IgniteClientConfiguration.java) it says "each corresponds to an active cluster node". Consider adding "active" for consistency and clarity, as not all IP addresses may correspond to active nodes.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/LogMessages.cs:
##########
@@ -267,4 +267,16 @@ internal static partial void LogTxStartedTrace(
Level = LogLevel.Trace,
EventId = 1036)]
internal static partial void LogTxRollbackTrace(this ILogger logger, long txId);
+
+ [LoggerMessage(
+ Message = "Error while re-resolving addresses: {Message}",
+ Level = LogLevel.Warning,
+ EventId = 1037)]
+ internal static partial void LogErrorWhileReResolvingDnsWarn(this ILogger logger, Exception e, string message);
+
+ [LoggerMessage(
+ Message = "Endpoints updated [added=[{Added}], removed=[{Removed}]]",
+ Level = LogLevel.Warning,
+ EventId = 1038)]
+ internal static partial void LogEndpointListUpdatedTrace(this ILogger logger, string added, string removed);
Review Comment:
The log level in the LoggerMessage attribute (Warning) doesn't match the method name suffix (Trace). The method is used with LogLevel.Trace check at line 446 in ClientFailoverSocket.cs, so the level should be LogLevel.Trace instead of LogLevel.Warning.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientFailoverSocket.cs:
##########
@@ -470,6 +557,42 @@ private async Task ConnectAllSockets()
}
}
+ private async Task ReResolveDnsPeriodically()
+ {
+ var interval = Configuration.Configuration.ReResolveAddressesInterval;
+
+ if (interval <= TimeSpan.Zero)
+ {
+ // Re-resolve is disabled.
+ return;
+ }
+
+ while (!_disposed)
+ {
+ await Task.Delay(interval).ConfigureAwait(false);
+ await ReResolveDns().ConfigureAwait(false);
+ }
+ }
+
+ [SuppressMessage(
+ "Microsoft.Design",
+ "CA1031:DoNotCatchGeneralExceptionTypes",
+ Justification = "Re-resolve errors are logged and skipped.")]
+ private async Task ReResolveDns()
+ {
+ try
+ {
+ // Skip if another operation is in progress.
+ await InitEndpointsAsync(lockWaitTimeoutMs: 1).ConfigureAwait(false);
+ }
+ catch (Exception e)
+ {
+ _logger.LogErrorWhileReResolvingDnsWarn(e, e.Message);
+ }
+ }
+
+ private void ScheduleReResolveDns() => ThreadPool.QueueUserWorkItem(o => _ = ReResolveDns());
Review Comment:
The ThreadPool.QueueUserWorkItem call should use the generic overload with state parameter and a static callback for consistency with other usage in the codebase (see ClientSocket.cs lines 72 and 850). This avoids unnecessary closure allocation. The lambda parameter 'o' is also unused.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/SocketEndpointComparer.cs:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Apache.Ignite.Internal;
+
+using System;
+using System.Collections.Generic;
+
+/// <summary>
+/// Equality comparer for <see cref="SocketEndpoint"/>.
+/// </summary>
+internal sealed class SocketEndpointComparer : IEqualityComparer<SocketEndpoint>
+{
+ /// <inheritdoc/>
+ public bool Equals(SocketEndpoint? x, SocketEndpoint? y)
+ {
+ if (ReferenceEquals(x, y))
+ {
+ return true;
+ }
+
+ if (x is null)
+ {
+ return false;
+ }
+
+ if (y is null)
+ {
+ return false;
+ }
+
+ if (x.GetType() != y.GetType())
+ {
+ return false;
+ }
Review Comment:
The GetType() check is unnecessary since SocketEndpoint is a sealed class (as seen in SocketEndpoint.cs line 27). This check can be removed to simplify the code.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26894 .NET: Improve DNS resolution logic [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #7261:
URL: https://github.com/apache/ignite-3/pull/7261#discussion_r2629763772
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientFailoverSocket.cs:
##########
@@ -385,6 +408,74 @@ internal IEnumerable<ClientSocket> GetSockets()
return res;
}
+ private async Task InitEndpointsAsync(int lockWaitTimeoutMs = Timeout.Infinite)
+ {
+ bool lockAcquired = await _socketLock.WaitAsync(lockWaitTimeoutMs).ConfigureAwait(false);
Review Comment:
Changed to a separate lock. We want to avoid multiple re-resolve operations running in parallel, so we need a lock for the entire duration of re-resolve. For example, when topology changes, we might receive multiple events of assignment change and socket disconnect rapidly. A lock with a timeout ensures we don't re-resolve too many times.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26894 .NET: Improve DNS resolution logic [ignite-3]
Posted by "isapego (via GitHub)" <gi...@apache.org>.
isapego commented on code in PR #7261:
URL: https://github.com/apache/ignite-3/pull/7261#discussion_r2627675014
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientFailoverSocket.cs:
##########
@@ -385,6 +408,74 @@ internal IEnumerable<ClientSocket> GetSockets()
return res;
}
+ private async Task InitEndpointsAsync(int lockWaitTimeoutMs = Timeout.Infinite)
+ {
+ bool lockAcquired = await _socketLock.WaitAsync(lockWaitTimeoutMs).ConfigureAwait(false);
Review Comment:
Why do you need to hold this lock for so long? You can at least release it before disposing old sockets.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientFailoverSocket.cs:
##########
@@ -385,6 +408,74 @@ internal IEnumerable<ClientSocket> GetSockets()
return res;
}
+ private async Task InitEndpointsAsync(int lockWaitTimeoutMs = Timeout.Infinite)
+ {
+ bool lockAcquired = await _socketLock.WaitAsync(lockWaitTimeoutMs).ConfigureAwait(false);
+ if (!lockAcquired)
+ {
+ return;
+ }
+
+ try
+ {
+ if (_disposed)
+ {
+ return;
+ }
+
+ HashSet<SocketEndpoint> newEndpoints = await GetIpEndPointsAsync(Configuration.Configuration).ConfigureAwait(false);
+ IReadOnlyList<SocketEndpoint> oldEndpoints = _endpoints;
+
+ var resList = new List<SocketEndpoint>(newEndpoints.Count);
+ List<SocketEndpoint>? toRemove = null;
+
+ // Retain existing sockets for endpoints that are still present.
+ foreach (var oldEndpoint in oldEndpoints)
+ {
+ if (newEndpoints.Remove(oldEndpoint))
+ {
+ resList.Add(oldEndpoint);
+ }
+ else
+ {
+ toRemove ??= new List<SocketEndpoint>();
+ toRemove.Add(oldEndpoint);
+ }
+ }
+
+ if (_logger.IsEnabled(LogLevel.Trace) && (newEndpoints.Count > 0 || toRemove != null))
+ {
+ var addedStr = newEndpoints.Select(e => e.EndPointString).StringJoin();
+ var removedStr = toRemove?.Select(e => e.EndPointString).StringJoin();
+
+ _logger.LogEndpointListUpdatedTrace(addedStr, removedStr ?? string.Empty);
+ }
+
+ // Add remaining endpoints that were not known before.
+ resList.AddRange(newEndpoints);
+
+ // Apply the new endpoint list.
+ _endpoints = resList;
+
+ // Dispose removed sockets after updating the endpoint list.
Review Comment:
Should we really disconnect from the nodes we don't resolve anymore? I thought we agreed on keeping them until they disconnect themselves?
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientFailoverSocket.cs:
##########
@@ -470,6 +557,43 @@ private async Task ConnectAllSockets()
}
}
+ private async Task ReResolveDnsPeriodically()
+ {
+ var interval = Configuration.Configuration.ReResolveAddressesInterval;
+
+ if (interval <= TimeSpan.Zero)
+ {
+ // Re-resolve is disabled.
+ return;
+ }
+
+ while (!_disposed)
+ {
+ await Task.Delay(interval).ConfigureAwait(false);
+ await ReResolveDns().ConfigureAwait(false);
+ }
+ }
+
+ [SuppressMessage(
+ "Microsoft.Design",
+ "CA1031:DoNotCatchGeneralExceptionTypes",
+ Justification = "Re-resolve errors are logged and skipped.")]
+ private async Task ReResolveDns()
+ {
+ try
+ {
+ // Skip if another operation is in progress.
+ await InitEndpointsAsync(lockWaitTimeoutMs: 1).ConfigureAwait(false);
Review Comment:
Why should we skip it? Why can not we just wait for the lock? It's a separate task anyway, it won't block anything. Can not it lead to a situation where we very rarely re-resolve list for busy clients and the user has no control over it?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26894 .NET: Improve DNS resolution logic [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #7261:
URL: https://github.com/apache/ignite-3/pull/7261#discussion_r2629765953
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientFailoverSocket.cs:
##########
@@ -385,6 +408,74 @@ internal IEnumerable<ClientSocket> GetSockets()
return res;
}
+ private async Task InitEndpointsAsync(int lockWaitTimeoutMs = Timeout.Infinite)
+ {
+ bool lockAcquired = await _socketLock.WaitAsync(lockWaitTimeoutMs).ConfigureAwait(false);
+ if (!lockAcquired)
+ {
+ return;
+ }
+
+ try
+ {
+ if (_disposed)
+ {
+ return;
+ }
+
+ HashSet<SocketEndpoint> newEndpoints = await GetIpEndPointsAsync(Configuration.Configuration).ConfigureAwait(false);
+ IReadOnlyList<SocketEndpoint> oldEndpoints = _endpoints;
+
+ var resList = new List<SocketEndpoint>(newEndpoints.Count);
+ List<SocketEndpoint>? toRemove = null;
+
+ // Retain existing sockets for endpoints that are still present.
+ foreach (var oldEndpoint in oldEndpoints)
+ {
+ if (newEndpoints.Remove(oldEndpoint))
+ {
+ resList.Add(oldEndpoint);
+ }
+ else
+ {
+ toRemove ??= new List<SocketEndpoint>();
+ toRemove.Add(oldEndpoint);
+ }
+ }
+
+ if (_logger.IsEnabled(LogLevel.Trace) && (newEndpoints.Count > 0 || toRemove != null))
+ {
+ var addedStr = newEndpoints.Select(e => e.EndPointString).StringJoin();
+ var removedStr = toRemove?.Select(e => e.EndPointString).StringJoin();
+
+ _logger.LogEndpointListUpdatedTrace(addedStr, removedStr ?? string.Empty);
+ }
+
+ // Add remaining endpoints that were not known before.
+ resList.AddRange(newEndpoints);
+
+ // Apply the new endpoint list.
+ _endpoints = resList;
+
+ // Dispose removed sockets after updating the endpoint list.
Review Comment:
Good catch. Fixed. Now we retain old connections, mark them as "abandoned", and remove them only when they get disconnected naturally.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-26894 .NET: Improve DNS resolution logic [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #7261:
URL: https://github.com/apache/ignite-3/pull/7261#discussion_r2629767176
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientFailoverSocket.cs:
##########
@@ -470,6 +557,43 @@ private async Task ConnectAllSockets()
}
}
+ private async Task ReResolveDnsPeriodically()
+ {
+ var interval = Configuration.Configuration.ReResolveAddressesInterval;
+
+ if (interval <= TimeSpan.Zero)
+ {
+ // Re-resolve is disabled.
+ return;
+ }
+
+ while (!_disposed)
+ {
+ await Task.Delay(interval).ConfigureAwait(false);
+ await ReResolveDns().ConfigureAwait(false);
+ }
+ }
+
+ [SuppressMessage(
+ "Microsoft.Design",
+ "CA1031:DoNotCatchGeneralExceptionTypes",
+ Justification = "Re-resolve errors are logged and skipped.")]
+ private async Task ReResolveDns()
+ {
+ try
+ {
+ // Skip if another operation is in progress.
+ await InitEndpointsAsync(lockWaitTimeoutMs: 1).ConfigureAwait(false);
Review Comment:
Please see my comment above (about a separate lock).
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org