Skip to content

Conversation

@Mpdreamz
Copy link
Member

This is still a work in progress but resembles a second refactor pass on IConnection after @icanhasjonas excellent feedback on #537 and earlier by @synhershko on #522

  • IConnection now only returns ElasticsearchResponse<Stream>
  • IConnection no longer in charged of deserialization, this is pushed back to Transport which is the coordinator between IConnectionPool and IConnection and already did the serialization.
  • <T> no longer leaks throughout all the components involved just to be able to deserialize in the deepest call in the stack.
  • IConnection no longer in charge of shortcircuiting for <string> <byte[]> or ignoring the stream completely for <VoidResponse>. A connection is now only in charge of returning a response stream and settings connection status data on the ElasticsearchResponse<Stream>, it should not know more of the internals.
  • As a result you can now do client.Call<Stream>() and do as you please with it, you're on your own (but please Dispose() it!)
  • If you do not specify a Stream return type Elasticsearch.Net still takes ownership of the Stream and disposes it properly.
  • ElasticsearchResponse<T> had an .Error property that hold some information and caught some exceptions. This is gone now. If a request succeeds to the point where we have a status code Elasticsearch.net will never throw.It will still expose the original exception (ie WebException) on .OriginalException any other exception will be rethrown (possibly caught again when an IConnectionPool is in place which will rethrow in a wrapped MaxRetryException when all nodes are tried).

Still todo:

  • Move Ping()and Sniff() from IConnection
  • ElasticsearchResponse<T> can now also return very interesting metric data (start time, time to first byte, time to last byte, (de)serialization time), will wait to explore that post alpha release.

@synhershko
Copy link

@Mpdreamz while at it, maybe standardize the Ok / IsValid / etc response codes? I'd throw an ElasticClientException if encountered an error, and obviously document that so consumers know they should watch for it, instead of doing TryConnect and then requiring to check a property for request / connection status.

Following that guideline, I'd also prefer to throw in case of errors instead of keeping the error in a property (old C-style dev)

…tate objects, can now control request specific parameters beyond just querystring
…d super'ed connection overrides into IRequestConnectionConfiguration
@Mpdreamz
Copy link
Member Author

Mpdreamz commented Apr 1, 2014

Elasticsearch prior to 1.0 had several different response codes (ok, acknowledged) etcetera, these have been standardized and the master of NEST has also removed all the Ok properties.

The TryConnect() and Isvalid constructs on the client have long been removed (0.12.0 already doesn't have them anymore), they we're the first things I wrote in 2010 and have regretted doing so ever since :)

I personally much prefer to not throw on http status codes other then 200-300 and possibly 404.

The clients can be configured to always throw though:

http://nest.azurewebsites.net/elasticsearch-net/errors.html

Although an easier way to do it i.e .ThrowOnFaultyHttpStatusCodes() might be handy.

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Apr 1, 2014

The later commits also removed Ping and Sniff from IConnection.

A single call can now override global settings i.e

client.Bulk("fixed-index", "fixed-type", data,.Bulk("fixed-index","fixed-type", r=>r
    .Refresh(true)
    .Consistency(ConsistencyOptions.All)
    .RequestConfiguration(c=>c
        .ConnectTimeout(50)
        .RequestTimeout(50)
        .DisableSniffing()
    )
)

Mpdreamz added a commit that referenced this pull request Apr 1, 2014
@Mpdreamz Mpdreamz merged commit f85ce38 into master Apr 1, 2014
@Mpdreamz Mpdreamz deleted the feature/simplify-iconnection branch June 17, 2014 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants