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

Remove 'utc' field from TimeSlicedOutput conf to prevent ConfigError in plugin helpers #1319

Merged
merged 4 commits into from
Nov 24, 2016

Conversation

repeatedly
Copy link
Member

@repeatedly repeatedly commented Nov 21, 2016

Helpers/Compat utils can't judge user specifies both localtime/utc or converted result.
This changes conf dumpped result but it seems acceptable.

Without this change, v0.12 TimeSlicedOutput plugins with utc parameter don't work.

Helpers/Compat utils can't judge user specifies both localtime/utc or converted result.
This changes conf dumpped result but it seems acceptable.
@repeatedly repeatedly force-pushed the fix-utc-handling-compat-timesliced-output branch from fd2d8f9 to 70aeaaa Compare November 21, 2016 12:14
@repeatedly
Copy link
Member Author

repeatedly commented Nov 21, 2016

This is incomplete. Incompatibility still exists by conf['timezone'] = "+0000".
v0.12 formatter doesn't generate "+0000" suffix time with utc.
Existing plugins assume formatted result is "Z" suffix time with utc.

@repeatedly
Copy link
Member Author

repeatedly commented Nov 21, 2016

@tagomoris Could you handle this problem? Currently, v0.14 has incompatibility issue with v0.12 TimeSlicedOutput.
S3 plugin has tests for thsese cases, so passed S3 tests may cover other TimeSlicedOutput plugin cases.

Here is one failed case:

Failure: test_format(S3OutputTest)
/Users/repeatedly/dev/fluentd/fluentd/lib/fluent/test/output_test.rb:139:in `block in run'
/Users/repeatedly/dev/fluentd/fluentd/lib/fluent/test/input_test.rb:124:in `block in run'
/Users/repeatedly/dev/fluentd/fluentd/lib/fluent/test/base.rb:71:in `run'
/Users/repeatedly/dev/fluentd/fluentd/lib/fluent/test/input_test.rb:123:in `run'
/Users/repeatedly/dev/fluentd/fluentd/lib/fluent/test/output_test.rb:124:in `run'
test/test_out_s3.rb:151:in `test_format'
     148:     d.expect_format %[2011-01-02T13:14:15Z\ttest\t{"a":1}\n]
     149:     d.expect_format %[2011-01-02T13:14:15Z\ttest\t{"a":2}\n]
     150: 
  => 151:     d.run
     152:   end
     153: 
     154:   def test_format_included_tag_and_time
<"2011-01-02T13:14:15Z\ttest\t{\"a\":1}\n2011-01-02T13:14:15Z\ttest\t{\"a\":2}\n"> expected but was
<"2011-01-02T13:14:15+00:00\ttest\t{\"a\":1}\n2011-01-02T13:14:15+00:00\ttest\t{\"a\":2}\n">

@repeatedly
Copy link
Member Author

@tagomoris I added test for this case. Now, test failed.

Failure: test: check formatted time compatibility with utc. Should Z, not +00:00(FluentOutputTest::TimeSlicedOutputTest::test emit)
/Users/repeatedly/dev/fluentd/fluentd/lib/fluent/test/output_test.rb:139:in `block in run'
/Users/repeatedly/dev/fluentd/fluentd/lib/fluent/test/input_test.rb:124:in `block in run'
/Users/repeatedly/dev/fluentd/fluentd/lib/fluent/test/base.rb:71:in `run'
/Users/repeatedly/dev/fluentd/fluentd/lib/fluent/test/input_test.rb:123:in `run'
/Users/repeatedly/dev/fluentd/fluentd/lib/fluent/test/output_test.rb:124:in `run'
/Users/repeatedly/dev/fluentd/fluentd/test/test_output.rb:261:in `block (2 levels) in <class:TimeSlicedOutputTest>'
     258:         time = Time.parse("2016-11-08 12:00:00 UTC").to_i
     259:         d.emit({"a" => 1}, time)
     260:         d.expect_format %[2016-11-08T12:00:00Z\ttest\t{"a":1,"time":"2016-11-08T12:00:00Z"}\n]
  => 261:         d.run
     262:       end
     263:     end
     264:   end
<"2016-11-08T12:00:00Z\ttest\t{\"a\":1,\"time\":\"2016-11-08T12:00:00Z\"}\n"> expected but was
<"2016-11-08T12:00:00+00:00\ttest\t{\"a\":1,\"time\":\"2016-11-08T12:00:00+00:00\"}\n">

@tagomoris
Copy link
Member

I pushed two commits to fix this problem.
@repeatedly Could you check these and merge if these are ok?

@tagomoris tagomoris assigned repeatedly and unassigned tagomoris Nov 24, 2016
@repeatedly
Copy link
Member Author

I tested the patch with s3 plugin test and it fixes broken tests.
Thanks for quick fix.

@repeatedly
Copy link
Member Author

And LGTM

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