Final load balancing review feedback (#1626)

Author: JamesNK

src/Grpc.Net.Client/Balancer/DnsResolver.cs

@@ -37,7 +37,7 @@ namespace Grpc.Net.Client.Balancer
     /// Note: Experimental API that can change or be removed without any prior notice.
     /// </para>
     /// </summary>
-    internal sealed class DnsResolver : AsyncResolver
+    internal sealed class DnsResolver : PollingResolver
     {
         // To prevent excessive re-resolution, we enforce a rate limit on DNS resolution requests.
         private static readonly TimeSpan MinimumDnsResolutionRate = TimeSpan.FromSeconds(15);
@@ -74,6 +74,7 @@ public DnsResolver(Uri address, int defaultPort, ILoggerFactory loggerFactory, T
             _logger = loggerFactory.CreateLogger<DnsResolver>();
         }
 
+        /// <inheritdoc />
         protected override void OnStarted()
         {
             base.OnStarted();

src/Grpc.Net.Client/Balancer/ISubchannelCallTracker.cs

@@ -22,6 +22,9 @@ namespace Grpc.Net.Client.Balancer
 {
     /// <summary>
     /// An interface for tracking subchannel calls.
+    /// <para>
+    /// Note: Experimental API that can change or be removed without any prior notice.
+    /// </para>
     /// </summary>
     public interface ISubchannelCallTracker
     {

…rpc.Net.Client/Balancer/AsyncResolver.cs …c.Net.Client/Balancer/PollingResolver.cssrc/Grpc.Net.Client/Balancer/AsyncResolver.cs renamed to src/Grpc.Net.Client/Balancer/PollingResolver.cs src/Grpc.Net.Client/Balancer/AsyncResolver.cs renamed to src/Grpc.Net.Client/Balancer/PollingResolver.cs

@@ -24,14 +24,17 @@
 namespace Grpc.Net.Client.Balancer
 {
     /// <summary>
-    /// An abstract base type for <see cref="Resolver"/> implementations that use asynchronous logic to resolve the <see cref="Uri"/>.
+    /// An abstract base type for <see cref="Resolver"/> implementations that use asynchronous polling logic to resolve the <see cref="Uri"/>.
     /// <para>
-    /// <see cref="AsyncResolver"/> adds a virtual <see cref="ResolveAsync"/> method. The resolver runs one asynchronous
+    /// <see cref="PollingResolver"/> adds a virtual <see cref="ResolveAsync"/> method. The resolver runs one asynchronous
     /// resolve task at a time. Calling <see cref="Refresh()"/> on the resolver when a resolve task is already running has
     /// no effect.
     /// </para>
+    /// <para>
+    /// Note: Experimental API that can change or be removed without any prior notice.
+    /// </para>
     /// </summary>
-    public abstract class AsyncResolver : Resolver
+    public abstract class PollingResolver : Resolver
     {
         // Internal for testing
         internal Task _resolveTask = Task.CompletedTask;
@@ -48,17 +51,17 @@ public abstract class AsyncResolver : Resolver
         protected Action<ResolverResult> Listener => _listener!;
 
         /// <summary>
-        /// Initializes a new instance of the <see cref="AsyncResolver"/>.
+        /// Initializes a new instance of the <see cref="PollingResolver"/>.
         /// </summary>
         /// <param name="loggerFactory">The logger factory.</param>
-        protected AsyncResolver(ILoggerFactory loggerFactory)
+        protected PollingResolver(ILoggerFactory loggerFactory)
         {
             if (loggerFactory == null)
             {
                 throw new ArgumentNullException(nameof(loggerFactory));
             }
 
-            _logger = loggerFactory.CreateLogger<AsyncResolver>();
+            _logger = loggerFactory.CreateLogger<PollingResolver>();
         }
 
         /// <summary>
@@ -100,7 +103,8 @@ protected virtual void OnStarted()
         /// <summary>
         /// Refresh resolution. Can only be called after <see cref="Start(Action{ResolverResult})"/>.
         /// <para>
-        /// This is only a hint. Implementation takes it as a signal but may not start resolution.
+        /// The resolver runs one asynchronous resolve task at a time. Calling <see cref="Refresh()"/> on the resolver when a
+        /// resolve task is already running has no effect.
         /// </para>
         /// </summary>
         public sealed override void Refresh()
@@ -152,9 +156,6 @@ private async Task ResolveNowAsync(CancellationToken cancellationToken)
         /// Resolve the target <see cref="Uri"/>. Updated results are passed to the callback
         /// registered by <see cref="Start(Action{ResolverResult})"/>. Can only be called
         /// after the resolver has started.
-        /// <para>
-        /// This is only a hint. Implementation takes it as a signal but may not start resolution.
-        /// </para>
         /// </summary>
         /// <param name="cancellationToken">A cancellation token.</param>
         /// <returns>A task.</returns>

src/Grpc.Net.Client/Balancer/Resolver.cs

@@ -60,11 +60,15 @@ public abstract class Resolver : IDisposable
 
         /// <summary>
         /// Refresh resolution. Can only be called after <see cref="Start(Action{ResolverResult})"/>.
+        /// The default implementation is no-op.
         /// <para>
         /// This is only a hint. Implementation takes it as a signal but may not start resolution.
         /// </para>
         /// </summary>
-        public abstract void Refresh();
+        public virtual void Refresh()
+        {
+            // no-op
+        }
 
         /// <summary>
         /// Releases the unmanaged resources used by the <see cref="LoadBalancer"/> and optionally releases

src/Grpc.Net.Client/Balancer/StaticResolver.cs

@@ -51,11 +51,6 @@ public override void Start(Action<ResolverResult> listener)
             // Send addresses to listener once. They will never change.
             listener(ResolverResult.ForResult(_addresses, serviceConfig: null, serviceConfigStatus: null));
         }
-
-        public override void Refresh()
-        {
-            // no-op
-        }
     }
 
     /// <summary>

test/Grpc.Net.Client.Tests/GrpcChannelTests.cs

@@ -841,11 +841,6 @@ public override void Start(Action<ResolverResult> listener)
             {
                 throw new NotImplementedException();
             }
-
-            public override void Refresh()
-            {
-                throw new NotImplementedException();
-            }
         }
 #endif
 

test/Shared/TestResolver.cs

@@ -30,7 +30,7 @@
 
 namespace Grpc.Tests.Shared
 {
-    internal class TestResolver : AsyncResolver
+    internal class TestResolver : PollingResolver
     {
         private readonly Func<Task>? _onRefreshAsync;
         private readonly TaskCompletionSource<object?> _hasResolvedTcs;