ignore cancelled events

[colinsurprenant]

Fixes #90

To benefit from performance improvement of the events list processing of the overridden multi_filter method which defers to the Java DateFilter.receive() method where it iterates through the passed events list, I think an acceptable solution is to simply ignore the cancelled events in the DateFilter.receive() loop. The returned events will include the original cancelled event but they will still keep their cancelled state so it should not have any impact downstream and at the same time not impact the DateFileter performance.

[colinsurprenant]

@jordansissel @jsvd what do you think?

[suyograo]

LGTM.

[suyograo]

@colinsurprenant can you bump version and CHANGELOG and we can get this in?

[jsvd]

can we add a test to this?

[jordansissel]

Change looks good. I'd also like a test for this.

[colinsurprenant]

@jsvd as per my description, yes, I wan to add tests to this before merging. @suyograo yup, will bump version and CHANGELOG before merging.

[colinsurprenant]

@jsvd @jordansissel @suyograo I changed the implementation to check for the cancelled events in the executeParsers() method and use a new IGNORED enum result. This makes all public methods respect the cancellation contract.

Added Java test and Ruby spec.

[jsvd]

I think I liked better when the detection of cancelled events was outside of executeParsers. mainly because, conceptually, a cancelled event shouldn't be parsed. That said, I confirmed tests and code work well

[colinsurprenant]

@jsvd the problem is that executeParsers() is a public method so it also has to correctly handle cancelled events and to stay consistent with the enum result passing I decided to introduce the IGNORED result. I am open to suggestions if you think there is a better way to implement it though ...

[colinsurprenant]

@jordansissel ok with this?

[jordansissel]

@colinsurprenant I'm hoping this implementation change is temporary, as I believe the plugin should never be given cancelled events anyway (this, in my opinion, is a bug in logstash that plugins can receive cancelled events)

LGTM

[colinsurprenant]

@jordansissel well, with the current filter design this was solvable either in multi_filter to respect the base filter class multi_filter contract or this way, which I chose, to maximize performance.

It is temporary because as we move forward with the Java API this will be rewritten once it lands. It will also change once we properly propagate/use the batch object.

I agree that plugins should not have to care for cancelled event.

[jsvd]

is the method public because of testing? I guess the java way would be to unit test receive instead, or create a ParserExecutor class with executeParsers public method? I'm not going to block this regardless, LGTM

[colinsurprenant]

@jsvd I actually didn't go as much as to question why executeParsers was public - I could have - but the goal here is not really to refactor or improve the code structure but to fix a bug. Ideally these should be addressed in two separate issues/PR, at least this is what we aim to do. But the fact is that it is public now and it is not documented what the method intent is either. Now that you bring it up, it probably was just for testing in which case it could have been made protected and, similar to what you suggested, a wrapper test class could have been used.

Now, for unit testing receive - the current test suite does not test the receive method, so I opted to stay consistent and it seemed reasonable to assume that if all tests focus on executeParsers and the cancelled event scenario is also tested in executeParsers then receive will be correct. Also, receive, for the cancelled event, IS tested with my added Ruby specs.

Here you have it, the whole reasoning behind all this, hope it makes sense.

[colinsurprenant]

thanks @jsvd @suyograo @jordansissel merged in master

[colinsurprenant]

published as v3.1.5