-
Notifications
You must be signed in to change notification settings - Fork 692
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Non-blocking Future #9
Comments
I guess the idea is you can pass in your own execution context if you think it might become a bottleneck. If we move to using a Promise that we complete using an action listener, then the bottleneck moves to the underlying elasticsearch client executor, right? If so, what's the advantage? I'm not against the idea of doing it this way, I just need to better understand the advantage. |
ES java client manages it's own thread pool. If we make a blocking call in a single client-wide |
I see that we are thinking about slightly different cases. |
In usual, when you send an async request via a transport client, it registers a callback on it's internal thread pool and the pool will notify when it's done. Additional requests will be handled in the same manner. However, by blocking request inside a shared execution context, additional ones won't be sent until the previous one is done and the thread pool will hold only one request at once like this: |
There should be Elastic4s client between 'User' and 'ExecutionContext'.. |
Yes I see what you mean. I'll follow up tonight with some conversion work, or you can start on a branch if you want. |
take a look over what I've done, feedback very much welcome. |
Thanks :) I'll send a following PR in a few days. |
Hi, I'm looking for an ES client and your DSL-based approach looks very promising. Thanks in advance :)
One thing makes me worry is about the current
execute
implementation.https://github.com/sksamuel/elastic4s/blob/master/src/main/scala/com/sksamuel/elastic4s/Client.scala#L39-L41
It takes per-client
ExecutionContext
and returns aFuture
by wrapping blockingactionGet()
with afuture {}
block. It doesn't block the current thread but actually blocks the dedicated one, so it might reduce overall throughput and be a bottleneck.Suggestion is attaching an
ActionListener[T]
using this and returning aFuture
from our ownPromise
, for example (not tested, just a concept):It'll make the API require an
ExecutionContext
for every request. If you accept this change I'll open a PR when time allows.The text was updated successfully, but these errors were encountered: