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

Fix out_file test with timezone #1546

Merged
merged 3 commits into from
Apr 21, 2017
Merged

Conversation

repeatedly
Copy link
Member

@repeatedly repeatedly commented Apr 19, 2017

@mururu Could you test this on your environment?

In v0.14, compat parameters handles utc parameter for parsers / formatters.
So if we want to use utc in buffer, need to specify timekey_use_utc in <buffer>.

@repeatedly repeatedly changed the title Fix out file test with timezone Fix out_file test with timezone Apr 19, 2017
@repeatedly repeatedly self-assigned this Apr 19, 2017
@repeatedly repeatedly requested a review from mururu April 19, 2017 17:43
@mururu
Copy link
Member

mururu commented Apr 19, 2017

Passed tests on my environment.
IIRC, v0.14 should work with v0.12's output configuration parameters? If so, we should fix the implementation of the compatibility layer but not tests? Otherwise, it looks good to me.

@repeatedly
Copy link
Member Author

repeatedly commented Apr 19, 2017

If so, we should fix the implementation of the compatibility layer but not tests?

Yeah, this is one idea. The problem is old out_file uses utc for both time slice and formatter.
In this result, hard to identify this parameter is for output or formatter.
We want to resolve this problem with v0.14.
Other idea is logging warning message like below.

'utc' parameter for output is deprecated and this parameter is used for formatter plugin. Use timekey_use_utc in <buffer> instead

@mururu
Copy link
Member

mururu commented Apr 19, 2017

That makes sense.
Another concern is that if a user explicitly set time_slice_format in addition to utc, then <buffer> timekey_use_utc true </buffer> is inserted automatically, right? It is confusing for me. Is it no problem?

@repeatedly
Copy link
Member Author

repeatedly commented Apr 20, 2017

then timekey_use_utc true is inserted automatically, right?

Yes. Maybe keeping existing behaviour is not good for both developers and users.
So this seems decision and documentation issue.
Currently, no report from users for out_file.
So earlier is better for it.

@repeatedly
Copy link
Member Author

Add warning message.

@mururu
Copy link
Member

mururu commented Apr 20, 2017

Makes sense. LGTM.

@repeatedly repeatedly merged commit 646c040 into master Apr 21, 2017
@repeatedly repeatedly deleted the fix-out_file-test-with-timezone branch May 24, 2017 06:31
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