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