Skipping Stringifying response

[synhershko]

In ElasticsearchResponse a stream of the response should be kept and passed directly to the JSON deserializer.

Large strings are never a good thing to hold in memory, and most real-world use cases involve large string response from ES.

[Mpdreamz]

The string Result is lazy

https://github.com/Mpdreamz/NEST/blob/master/src/Elasticsearch.Net/Domain/ElasticsearchResponse.cs#L29

The response is read completely into a byte[] though

Being lazy all the way down and exposing the stream will have side effects (i.e connections not being closed properly) if consumers do not close properly.

You must call either the Stream.Close or the HttpWebResponse.Close method to close the stream and release the connection for reuse. It is not necessary to call both Stream.Close and HttpWebResponse.Close, but doing so does not cause an error. Failure to close the stream will cause your application to run out of connections.

Instead of keeping the stream and returning it it might be helpful to be able to inject your ownobject ResponseStreamReader(Stream stream) in which case .ResultBytes is always null and .ResultObject is filled.

Thoughts? Naming? 👍

[synhershko]

Yeah, this crossed my mind, but clients should really be high performance or at least offer a mode for that.

I don't like this convention. A better and more .NET-ish solution would be to pass a Func for response handling where the provided default reads it to the end into byte[] or string.

But I'm pretty sure you can use Streams for deserialization and still close everything properly.

[Mpdreamz]

Yeah they can and they should close them properly. Its more to do with the lifespan of the stream. I don't image ElasticsearchResponse having a great enough lifespan but my gut feeling says i'd like to guarantee closing the stream as soon as possible.

I.e : we could also make sure the Stream is disposed in the destructor of ElasticsearchResponse but then this puts a design burden on how IConnection should behave. For example the ThriftConnection prealocates TSockets and if it has to expose streams it will have to do complicated householding in order to not reuse a tsocket which has not been read yet. This could also introduced timeouts because streams are read too late. Not worth the trade off IMO and so a hook is much preferred.

this is how the low level part of Elasticsearch.net works now

1. var result = do.a.call()
2. var objectIWant = result.Deserialize<T>(result.Stream? || results.Bytes);

in order to hook into the response handling you will have to get inside 1.

1. var result = do.a.call(responseHandler: (s)=>s.Deserialize<MyObject>());
2. var objectIwant = (MyObject)result.Result;

result.Result is in this case a boxed object which either holds a byte[] or the whatever the responsehandler returns. This make the API slightly uglier but atleast gives you to opportunity to do what works best for you

Another approach would be to have generic overloads of every low level call that returns an ElasticsearchResponse<T>

1. var result = do.a.call<MyObject>(responseHandler: (s)=>s.Deserialize<MyObject>());
2. var objectIwant = result.Result;

Would altleast get rid of the boxing but will cause the API to double in size.

[synhershko]

I'm afraid I will have to familiarize myself with the internals a bit more before making any useful comments here..

[Mpdreamz]

No problem I'm also using this to get a clearer picture for myself.

The master of NEST right now has two clients the typed NEST one and the low level Elasticsearch.Net one. NEST uses the Elasticsearch.Net client internally.

What i describe here is how the low level client currently works.

Default workflow

-client.call()
---connection
-----returns byte[]
-client returns response
-user can now either:
----result.Deserialize<T>() from byte[] array using configured
    IElasticsearchSerializer
----result.Response is a lazy dynamic that can also be used to explore the 
     result (i,e result.Response.hits.hits[0]._id)

Proposed workflow
-client.call()
---connection
-----<handle response to deserialize from stream to T>
-user now has result.Object available. 

The latter workflow is how the other official clients also work but they have no notion of T everything is an object in js or hash map in ruby. There is no consensus among json serializers what the default representation of json is in .NET. i.e If you ask for a IDictionary<string, object> you can get any of the following back Dictionary<string, JObject> Dictionary<string, object> or Dictionary<string, string> depending on which serializer you use. Even if there was consensus a Dictionary<string, object> is not what you'd want in C# anyway.

In order for the proposed workflow to work T will have to passed in at the top of the call chain.

Proposed workflow
-client.call<T>()
---connection
--------<handle response to deserialize of stream to T>

One of the major benefits of NEST is that it has modelled T so it will always have an appropriate T to pass to the low level client. Somebody who wants string/anonymous object in and out may not be best served by mandating a <T> so ideally i'd like to support both workflows.

Which most likely means supporting both client.call() and client.call<T>() in the low level Elasticsearch.Net client.

[Mpdreamz]

I'm inclined to only expose client.Call<T> if T is a string or byte[] the response is read and passed as either of the two otherwise the IElasticsearchSerializer is called to transform the stream into T.

[synhershko]

The problem is with reading the stream to byte[] or string only to read them again later when deserializing.

The way I would like to see NEST work (low level + integration with high level) is as follows:

-client writes a query (or any other request for that matter)
--query is translated to a string (ES's JSON representation + endpoint); that string is made available for debugging purposes.
--connection is made to endpoint
---string (probably as a Stream unless seriaized for debugging) is streamed out to the connection
---response received and held as an open Stream object, not processed yet
---ResponseProcessor object/Func is invoked on Stream
----Default impl will use JSON.NET to immediately deserialize the entire response to the appropriate Elasticsearch response using the Stream object (more effecient than stringifying and then deserializing). This impl will also allow us to close the stream properly.
----Other implementations (maybe an alternative default one) would deserialize into JObjects for easier traversal without having to predefine classes etc. Or combine both methods.
----JToken/JObject is basically the best way to represent JSON in .NET objects, way better than a Dictionary of objects.

I guess what I see can be written in short like this:

-client.call()
--connection
---ToJObjectResponseProcessor invoked on Stream to generate JObject
--Alternate processors can do other things, like deserialize to String for example
--Optional: return the Stream directly to the caller, explicitly giving him the responsibility of closing the stream (and handing the stream in a timely manner so connection doesn't time out)
-return ResponseProcessor.Result, whatever that was (ResponseProcessor can be a template abstract class)

-client.call<T>()
--connection
---StronglyTypedResponseProcessor invoked on Stream to generate a result for T
--Alternate processors can still be passed here as well, including the JObject or String ResponseProcessors
-return ResponseProcessor.Result

Obviously this needs to account for other connection methods like Thrift.

I'll be less worried about how other clients work, more about how to do this in a way that makes sense in .NET world. I don't think there's any client that has got it right at this point.

On that note, the current way the Linq provider is built makes writing queries quite hard. Yes it is strongly typed and all that, but it requires quite a lot of typing and attention to detail while typing. I think it should be completely reimagined to provide a much better experience, even if this means it follows less closely with the JSON/REST API (which can and should be exposed as well, but as a lower level way of writing queries).

[Mpdreamz]

We're almost on the same page on this 👍 The byte[]/string step in between has to go thats a given.

I imagine https://github.com/Mpdreamz/NEST/blob/master/src/Elasticsearch.Net/Serialization/IElasticsearchSerializer.cs taking a slightly different form to accomodate for this.

Should have a PR for this next week and will be sure to ping you before merging.

[synhershko]

I guess my point is you assume the response has to be deserialized. What if we could sometimes avoid this, e.g. in high write scenarios with optimistic concurrency where we are only interested in HTTP 200 (Ack) or HTTP 409 (Nack) and we don't care about the actual response?

I don't say this should be standard behavior, but it should at least be possible to do (or hack) even with the high level client. I've been using high throughput to justify this, but you get the point..