Suggestion -- Go back on the strongly typed IConnection implementation, and revert back to path instead of uri in methods, and then implement the connection pool as a decoration class

[icanhasjonas]

.. that way you can get rid of Sniff(), Ping() -> cleaner code
i.e

public class ConnectionPool : IConnection
{
    // pseudo.. 
    public ConnectionPool( IEnumerable<IConnection> connections )
    {
        _selector = new RandomSelector( connections );
        // ......
    }

    protected async Task<ElasticResponse> Execute( HttpMethod method, string path, string data )
    {
        // delegate the call to the connection
        var connection = await FindConnectionAsync();
        return await connection.Execute( method, path, data ); // Get, Post, .... 
    }

    protected virtual async Task<IConnection> FindConnectionAsync()
    {
        for( var i = 0; i < _settings.RetryCount; i++ ) {
            var connection = await _selector.MoveNext(); // could be implemented as an IEnumerator<Task<IConnection>>
            if( connection != null ) {
                return connection;
            }
            await Task.Delay( _settings.RetryWait );
        }
        throw new SomeExceptionAboutNotAbleToFindAcceptableConnection();
    }
}

... or.. you know, I could just do it for you ;-)

[icanhasjonas]

Also.. it would break less of your legacy users code if done correctly...

[Mpdreamz]

Hi @icanhasjonas

This was indeed the initial approach I went with.

I did not want a decorator holding actual IConnections since an actual IConnection implementation might do its own pooling of actual sockets (like the thriftconnection). The default HttpConnection leaves all that up to the operation system so can be new'ed as often as possible.

The Uri will have to be part of the method signature since in nest 0.12 the base uri never changed and IConnection could simply read it from IConnectionSettings this is no longer the case.

The reason they return strongly typed ElasticsearchResponse<T> now is that this gives the connection a chance to favour deserializing from the stream. IConnection could return a ElasticsearchResponse<Stream> and push the responsibility of calling the deserialization back to the decorator: https://github.com/elasticsearch/elasticsearch-net/blob/master/src/Elasticsearch.Net/Connection/Transport.cs this would also get rid of the deserializationState parameter on IConnection and move the shortcircuiting logic in one place thats definitely a win 👍 .

The reason I placed Ping and Sniff directly on IConnection is because they should ideally fail faster then regular Get/Post/Puts. I can only remove these by introducing an optional parameter controlling connect timeouts. This would still introduce breaking changes.

I say ideally but HttpWebRequest has no means of controlling DNS lookup times / true connect timeouts

http://msdn.microsoft.com/en-us/library/system.net.httpwebrequest.timeout(v=vs.110).aspx

A Domain Name System (DNS) query may take up to 15 seconds to return or time out. 
If your request contains a host name that requires resolution and you set Timeout 
to a value less than 15 seconds, it may take 15 seconds or more before a
 WebException is thrown to indicate a timeout on your request.

Would love to hear how the current implementation is breaking your existing code.

Super valueable feedback @icanhasjonas many thanks 👍

[icanhasjonas]

Cool cool and thanks for taking time with an awesome reply -- but I still propose the decorator is the right way to go, just for sake of argument

• Pull the pooling out of thrift and decorate with a PoolingConnectionDecorator():IConnection
• Assume all IConnection implementation has a single responsibility only
• Make it optional
• ..make stuff as AutoDiscoverConnection() and SniffingConnectionDecorator() etc and put that specific logic in them..
• Ping() and Sniff() wouldn't be a part of the IConnection but the outer controlling "pool controller" and therefore you would achieve the goal of fast failing etc..
• It could possible also be broken out completely from NEST.* and distributed as a plugin -- i.e, an additional NUGET for the different discovery methods, different pooling and load balancing methods etc.. it just makes your code so much simpler and smaller ;-).. an approach I sure appreciate.

URI vs PATH.. just having a path would make a lot more sense when pooling connections, and if you ever where to consider my above proposal, I would say it's essential..
You would essentially move the responsibility and configuration of the host + protocol to where it belongs, in the Connection Object. Makes for good maintainability etc..

Anyway -- Thanks again Mr Laarman

(Breaking code would be any implementation of IConnection, obviously, and ConnectionSettings new requiring an additional defaultIndex argument... I have more and I'll return with them in a separate issue)

[Mpdreamz]

A tad confusing but there's the high level connection pooling which is basically registering what nodes are alive and how to select a next one (IConnectionPool) and connection pooling at a resource level which in the case of HttpWebRequest is handled by the OS and in the case of ThriftConnection is handled internally by the IConnection implementation.

The responsibility for host is handled by IConnectionPool (alternative implementations can go in seperate nuget packages just fine already i believe) the choice for protocol is whatever IConnection you inject into the client. IConnection single responsibility is to take a uri/method and optional data and return a stream. The latter is not the case in master right now but this issue made me realize calling the serializer should be handled by ITransport. Sniff and Ping should also go from IConnection.

The coordination between IConnection and IConnectionPool is handled by ITransport all of which are overridable injectable parts.

I should also stress that this connection pooling is completely optional in the upcoming version. If you use the constructor on the client with just a base Uri it won't do any connection pooling / cluster failover.

The next version of Nest will be 1.0 and is intended to break a few things, in fact IElasticClient has been completely rewritten from scratch on top of that quite a few elasticsearch 1.0 changes will cause breaking changes anyway. The first commit from nest was in 2010 so 2014 makes a complete rewrite long overdue :) I would love to get an issue hearing about breaking changes I might miss in the release notes but I wont guarantee fixing any backwards compatibility issues.

The thanks are mine to extend by the way, feedback like this priceless!