[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request #1279: TINKERPOP-2288 Replace closed connections directly

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

FlorianHockmann commented on a change in pull request #1279: TINKERPOP-2288 Replace closed connections directly
URL: https://github.com/apache/tinkerpop/pull/1279#discussion_r409661134
 
 

 ##########
 File path: gremlin-dotnet/src/Gremlin.Net/Driver/ConnectionPool.cs
 ##########
 @@ -22,117 +22,156 @@
 #endregion
 
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Threading;
 using System.Threading.Tasks;
 using Gremlin.Net.Driver.Exceptions;
 using Gremlin.Net.Process;
+using Polly;
 
 namespace Gremlin.Net.Driver
 {
     internal class ConnectionPool : IDisposable
     {
         private const int ConnectionIndexOverflowLimit = int.MaxValue - 1000000;
         
-        private readonly ConnectionFactory _connectionFactory;
-        private readonly CopyOnWriteCollection<Connection> _connections = new CopyOnWriteCollection<Connection>();
+        private readonly IConnectionFactory _connectionFactory;
+        private readonly CopyOnWriteCollection<IConnection> _connections = new CopyOnWriteCollection<IConnection>();
+
+        private readonly ConcurrentDictionary<IConnection, byte> _deadConnections =
+            new ConcurrentDictionary<IConnection, byte>();
         private readonly int _poolSize;
         private readonly int _maxInProcessPerConnection;
         private int _connectionIndex;
         private int _poolState;
         private const int PoolIdle = 0;
         private const int PoolPopulationInProgress = 1;
 
-        public ConnectionPool(ConnectionFactory connectionFactory, ConnectionPoolSettings settings)
+        public ConnectionPool(IConnectionFactory connectionFactory, ConnectionPoolSettings settings)
         {
             _connectionFactory = connectionFactory;
             _poolSize = settings.PoolSize;
             _maxInProcessPerConnection = settings.MaxInProcessPerConnection;
-            PopulatePoolAsync().WaitUnwrap();
+            ReplaceDeadConnectionsAsync().WaitUnwrap();
         }
         
         public int NrConnections => _connections.Count;
 
-        public async Task<IConnection> GetAvailableConnectionAsync()
+        public IConnection GetAvailableConnection()
         {
-            await EnsurePoolIsPopulatedAsync().ConfigureAwait(false);
-            return ProxiedConnection(GetConnectionFromPool());
+            var connection = Policy.Handle<ServerUnavailableException>()
 
 Review comment:
   There we some comments for this line [on an earlier commit](https://github.com/apache/tinkerpop/commit/5b490d310d089a82ccb8bf2e66d770299d224773#r38237808). I just want to sum them up here so we can include them in the review process of the PR:
   @idzmitry mentioned that adding a new dependency on Polly could be avoided by implementing the retry policy by ourselves.
   @spzSource suggested that it could make sense to add a random jitter to the exponential back-off here to avoid overloading the server when multiple clients try to reconnect at exactly the same intervals.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services