[GitHub] [pulsar-dotpulsar] goldenccargill commented on a diff in pull request #104: add server timeout functionality plus docker test

[GitBox <gi...@apache.org>]

goldenccargill commented on code in PR #104:
URL: https://github.com/apache/pulsar-dotpulsar/pull/104#discussion_r888819170


##########
src/DotPulsar/Abstractions/IPulsarClientBuilder.cs:
##########
@@ -53,6 +53,16 @@ public interface IPulsarClientBuilder
     /// </summary>
     IPulsarClientBuilder KeepAliveInterval(TimeSpan interval);
 
+    /// <summary>
+    /// The maximum amount of time to wait without receiving any message from the server at
+    /// which point the connection is assumed to be dead or the server is not responding.
+    /// As we are sending pings the server should respond to those at a minimum within this specified timeout period.
+    /// Once this happens the connection will be torn down and all consumers/producers will enter
+    /// the disconnected state and attempt to reconnect
+    /// The default is 60 seconds.
+    /// </summary>
+    IPulsarClientBuilder ServerResponseTimeout(TimeSpan interval);

Review Comment:
   don't know about other clients but it seems it is configurable on the broker so the client setting should agree with that so I think it needs to be configurable here. Shame the value isn't returned from the server as part of the connection response



##########
src/DotPulsar/Internal/PingPongHandler.cs:
##########
@@ -25,29 +25,37 @@ public sealed class PingPongHandler : IAsyncDisposable
 {
     private readonly IConnection _connection;
     private readonly TimeSpan _keepAliveInterval;
+    private readonly TimeSpan _serverResponseTimeout;
     private readonly Timer _timer;
     private readonly CommandPing _ping;
     private readonly CommandPong _pong;
     private long _lastCommand;
+    private readonly TaskCompletionSource<object> _serverNotRespondingTcs;

Review Comment:
   seems it has been removed
   https://github.com/StephenCleary/AsyncEx/issues/176



##########
src/DotPulsar/Internal/PingPongHandler.cs:
##########
@@ -61,13 +69,25 @@ private void Watch(object? state)
             var lastCommand = Interlocked.Read(ref _lastCommand);
             var now = Stopwatch.GetTimestamp();
             var elapsed = TimeSpan.FromSeconds((now - lastCommand) / Stopwatch.Frequency);
-            if (elapsed >= _keepAliveInterval)
+
+            if (elapsed > _serverResponseTimeout)
             {
-                Task.Factory.StartNew(() => SendPing());
-                _timer.Change(_keepAliveInterval, TimeSpan.Zero);
+                DotPulsarMeter.ServerTimedout();
+                _serverNotRespondingTcs.SetResult(new object());

Review Comment:
   done



-- 
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: dev-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org