You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2020/04/16 15:32:23 UTC
[GitHub] [tinkerpop] FlorianHockmann opened a new pull request #1279:
TINKERPOP-2288 Replace closed connections directly
FlorianHockmann opened a new pull request #1279: TINKERPOP-2288 Replace closed connections directly
URL: https://github.com/apache/tinkerpop/pull/1279
https://issues.apache.org/jira/browse/TINKERPOP-2288
This is still WIP but I think that a PR makes sense to be able to discuss the concrete implementation. There were already some comments on the earlier commit which are now unfortunately lost as I force pushed on that branch after rebasing on `3.4-dev` again.
Closed connections are now replaced automatically in the background.
If no open connection is available to answer a request, then the pool
tries it again after some time. It uses a retry policy with exponential
backoff for that, implemented with Polly.
This change also ensures that only one task performs a pool resizing
operation at a time.
These changes should ensure that:
- A connection is still returned quickly if one is available.
- Closed connections are replaced immediately, without needing to wait
for the next incoming request.
- If the server is only unavailable temporarily (or it just closed
open connections for some reason), then the user should should not get
an exception.
He only has to wait until the connections are replaced.
TODO:
- [ ] Make the retry policy configurable
- [ ] Document changes
----------------------------------------------------------------
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
[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request
#1279: TINKERPOP-2288 Replace closed connections directly
Posted by 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
[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request
#1279: TINKERPOP-2288 Replace closed connections directly
Posted by 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_r409664074
##########
File path: gremlin-dotnet/src/Gremlin.Net/Driver/ConnectionPool.cs
##########
@@ -148,26 +187,39 @@ private void ProtectIndexFromOverflowing(int currentIndex)
Interlocked.Exchange(ref _connectionIndex, 0);
}
- private void RemoveConnectionFromPool(Connection connection)
+ private void ReplaceConnection(IConnection connection)
{
- if (_connections.TryRemove(connection))
- DefinitelyDestroyConnection(connection);
+ RemoveConnectionFromPool(connection);
+ TriggerReplacementOfDeadConnections();
}
- private IConnection ProxiedConnection(Connection connection)
+ private void RemoveConnectionFromPool(IConnection connection)
{
- return new ProxyConnection(connection, ReturnConnectionIfOpen);
+ _deadConnections.TryAdd(connection, 0);
}
- private void ReturnConnectionIfOpen(Connection connection)
+ private void TriggerReplacementOfDeadConnections()
{
- if (connection.IsOpen) return;
- ConsiderUnavailable();
+ ReplaceClosedConnectionsAsync().Forget();
}
- private void ConsiderUnavailable()
+ private async Task ReplaceClosedConnectionsAsync()
{
- CloseAndRemoveAllConnectionsAsync().WaitUnwrap();
+ var poolWasPopulated = await EnsurePoolIsHealthyAsync().ConfigureAwait(false);
+ // Another connection could have been removed already, check if another population is necessary
+ if (poolWasPopulated)
+ await ReplaceClosedConnectionsAsync().ConfigureAwait(false);
Review comment:
There was also a comment on this line [in the earlier commit](https://github.com/apache/tinkerpop/commit/5b490d310d089a82ccb8bf2e66d770299d224773#r38325980) where @spzSource asked whether this recursive call could lead to a stack overflow. I answered also inline at that commit with an explanation of why I think that a stack overflow cannot happen here.
----------------------------------------------------------------
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