Fix/use thread locals #77
Conversation
|
@andrewvc Could you take a look? This will unblock @gingerwizard |
|
Is it possible to control the number of connections used by each client? Can we pass this as an option? This would atleast allow us to have some control, if somewhat abstracted, over the number of connections to ES. @ph |
|
It seems it uses a connection per host so this seems unlikely without a custom connection strategy. |
Well IIRC how the ruby client handle his pool, this PR change will make 1 connection per worker? Every call to the ES host are sync in the current context. |
|
I'm not too worried about conn usage. ES is pretty about handling high numbers of conns. Yes, in some cases it could be a problem. Unfortunately, we have no short-term options to fix the concurrency bug and limit connection usage. Fixing this stuff is definitely on our roadmap however. |
| :logger => @logger | ||
| } | ||
| @client = LogStash::Filters::ElasticsearchClient.new(@user, @password, options) | ||
| @clients_pool = {} |
There was a problem hiding this comment.
This isn't used. We should, however, use this as opposed to Thread.current[:filter_elasticsearch_client] since we'll need to close all the clients on pipeline shutdown.
I recommend making this a java.util.concurrent.ConcurrentHashMap indexed by Thread.current, and using that to handle the close behavior.
There was a problem hiding this comment.
Funny, It was my first draft yesterday. I've figured since we already have Thread.current's hash it would have been enough.
Since the ES Ruby client isn't thread safe, we are currently using Thread local to create a client per workers to make the connection pool thread safe. closes: logstash-plugins#76
ph
force-pushed
the
fix/use-thread-locals
branch
from
a072db6
to
93907bf
Compare
|
@andrewvc updated with your review, travis still seems to be on the fritz. |
|
I don't see any close behavior in these changes, I think we will need to
close the clients
…On Sep 15, 2017 9:32 AM, "Pier-Hugues Pellerin" ***@***.***> wrote:
@andrewvc <https://github.com/andrewvc> updates with your review.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#77 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAIBYztTx-mn7o_8PLOcWOb_Yhx2XYOAks5siop8gaJpZM4PWpgB>
.
|
|
@andrewvc When I did the PR, I've looked at the elasticsearch-ruby documentation and the code. The client and the connection class doesn't expose any way to close the connection. By looking at the factory https://github.com/logstash-plugins/logstash-filter-elasticsearch/blob/master/lib/logstash/filters/elasticsearch/client.rb#L28 We are using the default transport which should be net/http with Faraday. Complete adapter code is at https://github.com/lostisland/faraday/blob/master/lib/faraday/adapter/net_http.rb, the library is using the block syntax, so It should open/close on every request. Even if we were using the manticore adapter that we have developed it doesn't have a close implementation. Next steps?
|
|
@ph thanks for the investigation. That's really bad news about the implementation, but I guess there's nothing to do here. We'll fix that when we move to using the ES output's client |
| end | ||
|
|
||
| def get_client | ||
| @clients_pool.computeIfAbsent(Thread.current, ->(k) { new_client }) |
There was a problem hiding this comment.
This is ruby 2.0 syntax, so it won't work with logstash < 6. Is that intentional?
There was a problem hiding this comment.
We don't good catch, I am already used to the new koolaid.
The elasticsearch ruby client isn't thread safe
Since the ES Ruby client isn't thread safe, we are currently using
Thread local to create a client per workers to make the connection pool
thread safe.
closes: #76