search

tagomoris / fluent-plugin-parser

Other
74 stars 23 forks source link

Remove custom parser #15

Closed fujiwara closed 9 years ago

fujiwara commented 10 years ago

One incompatibility change is included.

When input stream includes invalid time value (e.g. {"time":[]}), old implementation pass through time value. New implementation handles it as error because Fluent::TextParser raise an exception.

tagomoris commented 10 years ago

I think that that we cannot help that incompatibility because Fluent::TextParser should raise any specific exception like Fluent::TextParser::TimeParseError or so, but doesn't. There are 2 options:

Raising exceptions on parser seems bad for me because input plugins cannot do anything if input plugin fails to emit by parser's exception. So I think that the 1st option is better than 2nd.

tagomoris commented 10 years ago

Furthermore, we should make pull request for Fluentd to fix to raise any explicit exception for errors of TimeParser.

tagomoris commented 9 years ago

@fujiwara All required changes are included in b5f2636?

fujiwara commented 9 years ago

Exceptions handling is not yet. I'm considering how to handle exceptions at parsing invalid format date string(or object or...). But, I can't decide it. because raised exceptions will be many types by input record's value.

https://twitter.com/fujiwara/status/507702562187796480

tagomoris commented 9 years ago

How about to do #to_s for non- string/numeric values in time field?

lancechentw commented 9 years ago

Hello, anything new here?

tagomoris commented 9 years ago

We've waited for this fix: https://github.com/fluent/fluentd/commit/78f045d86f4341be287eaf75dfa1a103b4944a65

And now, we can use this fix after v0.10.54, so it is time to proceed this pull request. @Lance0312 Thank you for good notification. @fujiwara Do you have time to do it?

tagomoris commented 9 years ago

See also: https://github.com/fluent/fluentd/pull/453

lancechentw commented 9 years ago

@tagomoris Thank you for the updates. :+1:

fujiwara commented 9 years ago

I've fixed all problems. Incompatibility changes from current version:

tagomoris commented 9 years ago

:+1: I'll check code later.

tagomoris commented 9 years ago

LGTM! I'll add some tests for custom parser plugin for fluentd's TextParser after this merge.

lancechentw commented 9 years ago

@fujiwara @tagomoris :+1: Thank you!

tagomoris commented 9 years ago

Merged to master, and released v0.4.0! Thank you @fujiwara !