logstash-plugins / logstash-filter-translate

Translate filter for Logstash
Apache License 2.0
21 stars 47 forks source link

Update code, use Rufus scheduler, load dictionary better, add "iterate_on" #67

Closed guyboertje closed 6 years ago

guyboertje commented 6 years ago
colinsurprenant commented 6 years ago

@guyboertje did a first pass review. Great refactor overall 👍 Have we made any memory leaks testing to make sure reloading/refreshing dictionaries does not leak?

guyboertje commented 6 years ago

No memory leak testing done as yet.

colinsurprenant commented 6 years ago

@guyboertje it shouldn't be too hard to write dictionary reloading stress tests which should reveal quickly if there are any memory leaks. We could create a separate followup issue for this. I am focusing on this because this is a very typical place where such memory leaks happens.

colinsurprenant commented 6 years ago

LGTM

ccayg-sainsburys commented 6 years ago

https://github.com/logstash-plugins/logstash-filter-translate/pull/67/files#diff-bf70891427c2690568e84ec2c794d12dR4 (require "logstash/util/loggable")

Is, I believe, breaking the functionality of this plugin for us - auto tests which were working now broken with no related change and the following error when trying to start the logstash service:

Couldn't find any filter plugin named 'translate'. Are you sure this is correct? Trying to load the translate filter plugin resulted in this error: no such file to load -- logstash/util/loggable

I presume there is now a dependency that I need to load? But I'm not clear what?

I see this is also now: https://github.com/logstash-plugins/logstash-filter-translate/issues/69