Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LabeledTSVParser error handling and default parameters. #124

Merged
merged 4 commits into from
May 1, 2013

Conversation

y-ken
Copy link
Member

@y-ken y-ken commented Apr 29, 2013

ValuesParser is not delete the output of time key like other do so.

Time key should be deleted when it has used for fluentd internal time like TSVParser, CSVParser do so.

LabeledTSVParser error handling problem

The error of "time_format parameter is ignored" puts message to $log.warn.
It should raise ConfigError.

The default of time_key for LabeledTSVParser

I think LabeledTSVParser should be 'time' for the default of time_key.
This behavior causes confusion and not same as ApacheParser, JSONParser and RegexpParser.
It cannot seamless upgrade from in_tail with regex or apache apache2 nginx and so on.
There configuration usualy have time_format but not it has time_key. like http://ltsv.org/
If it not 'time' for the default of time_key, it could result using current time when time_format is blank.

footnote
ValuesParser was used by LabeledTSVParser, TSVParser, CSVParser

@ghost ghost assigned repeatedly Apr 29, 2013
@repeatedly
Copy link
Member

LTSVParser always use current time when time_key wasn't set even thouth set time_format.

This seems ValuesParser behaviour, not LTSVParser.
But your last commit fixes this problem I think, right?

@y-ken
Copy link
Member Author

y-ken commented May 1, 2013

@repeatedly
I have update issue. Would you please check it again?

repeatedly added a commit that referenced this pull request May 1, 2013
The Problems I have found for lib/fluent/parser.rb
@repeatedly repeatedly merged commit fedb7f8 into fluent:master May 1, 2013
@repeatedly
Copy link
Member

Okay, merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants