Add filter_with_time to Fluent::Plugin::Filter
#1140
Conversation
ganmacs
force-pushed
the
add-new-method-to-filter
branch
3 times, most recently
from
f253383
to
7cb5208
Compare
| end | ||
| end | ||
|
|
||
| def implemented_method_name |
There was a problem hiding this comment.
This process (detecting implemented methods) should be done as early as possible. Lazy call (the first time when events arrived) makes data loss if the filter was implemented in invalid way.
IMO it's best to do it in #initialize. Raising error in #initialize makes fluentd to stop at the time just launched.
There was a problem hiding this comment.
Because we can control a timing of raising an error and it is useful for test etc ,I added a code that check which method(filter or filter_with_time) implemented at #configure
Sorry, I misunderstood. I added add this method call to #initialize
https://github.com/fluent/fluentd/pull/1140/files#diff-3bdc55bd92825d185fcb6fc41334cef3R35
ganmacs
force-pushed
the
add-new-method-to-filter
branch
9 times, most recently
from
6e933ce
to
ff34788
Compare
| when [:filter, :filter_with_time].all? { |e| implmented_methods.include?(e) } | ||
| raise "BUG: Filter plugins MUST be implemented either `filter` or `filter_with_time`" | ||
| when implmented_methods.include?(:filter) | ||
| :filter |
There was a problem hiding this comment.
This seems verbose. true / false is enough for me.
There was a problem hiding this comment.
ganmacs
force-pushed
the
add-new-method-to-filter
branch
3 times, most recently
from
2b1bae7
to
3db34c4
Compare
| # Plugins that override `filter_stream` don't need check, | ||
| # because they may not call `filter` or `filter_with_time` | ||
| # for example fluentd/lib/fluent/plugin/filter_record_transformer.rb | ||
| return if implmented_methods.include?(:filter_stream) |
There was a problem hiding this comment.
Returning without any value looks not good manner for this method... What is set into @has_filter_with_time??
It's better to return nil or something else explicitly.
Code comment looks very good.
There was a problem hiding this comment.
|
@ganmacs Could you fix just one more point? |
this is for plugings that updates times when filtering
Avoid to call heavy method call in loop
ganmacs
force-pushed
the
add-new-method-to-filter
branch
from
3db34c4
to
ed21e9a
Compare
Plugins that override `filter_stream` don't need check, because they may not call `filter` or `filter_with_time`
ganmacs
force-pushed
the
add-new-method-to-filter
branch
from
ed21e9a
to
0353283
Compare
tagomoris
added
feature request
|
Great addition. Thank you! |
This is for plugins that updates times when filtering.
ref: #1139