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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
