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.

Sign up for GitHub

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

Improve flush_interval handling #1442

Merged
merged 3 commits into from
Feb 17, 2017
Merged

Improve flush_interval handling #1442

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a8d0af
Set 'interval' to flush_mode when v0.12 config has flush_interval
repeatedly Jan 31, 2017
7187770
Log warning when flush_interval is ignored
repeatedly Jan 31, 2017
3a7ed6d
Change flush_interval check mechanizm and add more tests
repeatedly Feb 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/fluent/plugin/output.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,10 @@ def configure(conf)
log.warn "'flush_at_shutdown' is false, and buffer plugin '#{buf_type}' is not persistent buffer."
log.warn "your configuration will lose buffered data at shutdown. please confirm your configuration again."
end

if @flush_mode != :interval && buffer_conf.has_key?('flush_interval')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks better to set :interval when flush_mode is :default, even when timekey is specified, here:
https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/output.rb#L292
We need to know flush_interval is set in <buffer> section or not before setting :interval... how about to add has_flush_interval before super as same with has_buffer_section?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it's better to warn when both of flush_interval and flush_mode lazy(or immediate) are configured explicitly by users (these are exclusive parameters).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks better to set :interval when flush_mode is :default

https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/output.rb#L293

From above line, :default is changed to :lazy or :interval so :default is not appeared here, right?

how about to add has_flush_interval before super as same with has_buffer_section?

It seems good.

these are exclusive parameters

If so, should we raise an ConfigError when flush_interval and flush_mode lazy are set explictly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean to set @flush_mode = :interval at L293 when flush_interval is explicitly specified in configuration even if @chunk_key_time is true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we doesn't take care about compatibility layer, it's better to raise configuration errors when both of flush_mode lazy and flush_interval are specified in configuration file.
But compatibility layer may specify these both if a plugin is TimeSlicedOutput and flush_interval exists in configuration. So we can't raise errors for such cases. warn is better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flush_interval is explicitly specified in configuration even if @chunk_key_time is true.

This means we should use :interval with following configuration?

<buffer>
  flush_interval 10s # use flush_mode interval
</buffer>

And warning for follwing configuration?

<buffer>
  flush_interval 10s
  flush_mode lazy
</buffer>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1st example: No. It depends on default of plugins, and output.rb should warn about ignored flush_interval when plugins' default is not interval.

My first point is that we cannot know the :interval set in @flush_mode is from plugin default or configuraitons at L314 (because it can be set at L293). So we should check conf before super to show appropriate warning.

2nd example: No. IMO it's better to raise error for inconsistent configuration.

The last my comment was wrong, because Fluent::Compat::TimeSlicedOutput#configure will set flush_mode as interval if conf has flush_interval. So we can raise exception if both of flush_mode lazy and flush_interval ... are specified in configurations explicitly.

log.warn "'flush_interval' is ignored because 'flush_mode' is not 'interval': '#{@flush_mode}'"
end
end

if @secondary_config
Expand Down
3 changes: 3 additions & 0 deletions lib/fluent/plugin_helper/compat_parameters.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,9 @@ def compat_parameters_buffer(conf, default_chunk_key: '')
hash['timekey'] = 86400 # TimeSliceOutput.time_slice_format default value is '%Y%m%d'
end
end
if conf.has_key?('flush_interval')
hash['flush_mode'] = 'interval'
end

e = Fluent::Config::Element.new('buffer', chunk_key, hash, [])
conf.elements << e
Expand Down
5 changes: 5 additions & 0 deletions test/plugin/test_output.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,11 @@ def waiting(seconds)
i.stop; i.before_shutdown; i.shutdown; i.after_shutdown; i.close; i.terminate
end

test 'flush_interval is ignored when flush_mode is not interval' do
mock(@i.log).warn("'flush_interval' is ignored because 'flush_mode' is not 'interval': 'lazy'")
@i.configure(config_element('ROOT', '', {}, [config_element('buffer', 'time', {'timekey' => 60*30, 'flush_interval' => 10})]))
end

test "Warn if primary type is different from secondary type and either primary or secondary has custom_format" do
o = create_output(:buffered)
mock(o.log).warn("secondary type should be same with primary one",
Expand Down
2 changes: 2 additions & 0 deletions test/plugin_helper/test_compat_parameters.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ def write(chunk); end # dummy
assert_equal [], @i.buffer_config.chunk_keys
assert_equal 8, @i.buffer_config.flush_thread_count
assert_equal 10, @i.buffer_config.flush_interval
assert_equal :interval, @i.buffer_config.flush_mode
assert @i.buffer_config.flush_at_shutdown

assert_equal 8*1024*1024, @i.buffer.chunk_limit_size
Expand All @@ -131,6 +132,7 @@ def write(chunk); end # dummy
assert @i.buffer_config.retry_forever
assert_equal 60*60, @i.buffer_config.retry_max_interval
assert_equal :block, @i.buffer_config.overflow_action
assert_equal :default, @i.buffer_config.flush_mode

assert [email protected]_key_tag
assert_equal [], @i.chunk_keys
Expand Down