search

logstash-plugins / logstash-filter-math

This plugin provides the ability to do various simple math operations (addition, subtraction, multiplication and division) on document fields
Apache License 2.0
1 stars 8 forks source link

Add threadsafety, create new register for each filter call (was: via register array per thread + ConcurrentHash) #10

Closed guyboertje closed 5 years ago

guyboertje commented 5 years ago

No version bump yet, one more change to come

guyboertje commented 5 years ago

@jsvd This is ready for master.

No version bump yet, one more change to come

Sorry, I meant in another PR.

colinsurprenant commented 5 years ago

changes LGTM - just need to fix the version bump. I suggest 2 commits: 1 for the changes (which does not include the changelog changes) and another with the version bump with both the changelog and gemspec changes.

guyboertje commented 5 years ago

@colinsurprenant Thanks for the review. I wanted to merge to master without a version bump.

I plan to solve this have a patch or not for this issue before release. If no action on issue 11 then I'll do another PR for release.

The next release will go in 6.5.x so we have time. Eager users can still use any release as it comes along. I have to do a blog post about this in due course.

colinsurprenant commented 5 years ago

@guyboertje we should keep version-bumb-less PRs only for those trivial and or cosmetic changes. I think this PR should get a proper version bump.

guyboertje commented 5 years ago

Normally, I agree 100%. But this plugin has not been included in a LS official release yet - so I think it is OK to merge now. #SpecialCase

guyboertje commented 5 years ago

A added a test that failed when it was pasted into math_spec.rb on the master branch.

colinsurprenant commented 5 years ago

LGTM - thread safety and potential memory leak raised by @yaauie should be solved here. I would still suggest to bump version and release this plugin with these changes, they are totally worthy of a version bump and release and every can benefit the bug fixes right away by upgrading.