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

Add buf_file_single plugin #2579

Merged
merged 13 commits into from
Aug 20, 2019
Merged

Add buf_file_single plugin #2579

merged 13 commits into from
Aug 20, 2019

Conversation

repeatedly
Copy link
Member

Which issue(s) this PR fixes:
Ref #2153

What this PR does / why we need it:
buf_file works fine for typical cases, e.g. in_tail/in_forward, but it is not good for lots of small emit cases. It issues lots of IO operation.
So to reduce IO tps on high traffic environment, introduce buf_file_single. This is similar to v0.12 buf_file and this is lightweight.

buf_file_single has one limitation that supports only tag or one field chunk key, <buffer tag> or <buffer key>. This limitation will be relaxed by supporting metadata header in the file.

Docs Changes:
Add buf_file_single article. Will send a patch later.

Release Note:
Same as title

Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
@repeatedly repeatedly added the feature request *Deprecated Label* Use enhancement label in general label Aug 19, 2019
@repeatedly repeatedly self-assigned this Aug 19, 2019
lib/fluent/plugin/buf_file_single.rb Outdated Show resolved Hide resolved
lib/fluent/plugin/buf_file_single.rb Show resolved Hide resolved
lib/fluent/plugin/buf_file_single.rb Outdated Show resolved Hide resolved
test/plugin/test_buf_file_single.rb Show resolved Hide resolved

@p.start
assert File.exist?(@bufdir)
assert { File.stat(@bufdir).mode.to_s(8).end_with?('755') }
Copy link
Member

Choose a reason for hiding this comment

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

assert_match? or assert_equal 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.

assert_equal is not fit because result is 40755 like string. This equal seems good for test.

end
end

def restore_metadata
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't file_single_chunk support file_chunk's metadata restore?(this is just confirmation)

Copy link
Member Author

Choose a reason for hiding this comment

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

We can support it with metadata header in the future.
Just now, support only one field key or tag.

test/plugin/test_buffer_file_single_chunk.rb Outdated Show resolved Hide resolved
test/plugin/test_buf_file_single.rb Outdated Show resolved Hide resolved
test/plugin/test_buffer_file_single_chunk.rb Outdated Show resolved Hide resolved
assert_equal (d1.to_json + "\n" + d2.to_json + "\n"), File.read(@c.path)
end

test 'can #rollback to revert non-committed data from #concat' do
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to add a test case for 'can not #rollback a committed data'

Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Copy link
Member

@ganmacs ganmacs left a comment

Choose a reason for hiding this comment

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

LGTM (though I left some comments.)

@d = create_driver
p = @d.instance.buffer
assert_equal File.join(@bufdir, 'fsb.*.buf'), p.path
end

data('text based chunk' => [FluentPluginFileSingleBufferTest::DummyOutputPlugin, :text],
'msgpack based chunk' => [FluentPluginFileSingleBufferTest::DummyOutputMPPlugin, :msgpack])
Copy link
Member

Choose a reason for hiding this comment

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

[nits] indent

desc 'The permission of chunk file. If no specified, <system> setting or 0644 is used'
config_param :file_permission, :string, default: nil # '0644'
desc 'The permission of chunk directory. If no specified, <system> setting or 0644 is used'
config_param :dir_permission, :string, default: nil # '0755'
Copy link
Member

Choose a reason for hiding this comment

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

L48 says 0644 but this line says 0755. which is one correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, forgot it. Will fix.


@state = :queued
@bytesize = @chunk.size
@commit_position = @chunk.size
Copy link
Member

Choose a reason for hiding this comment

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

Are there any reasons not to use @chunk.pos?

return nil unless res

key = base[4..res - 1] # remove 'fsb.' and '.'
hex_id = base[res + 2..-5] # remove '.' and '.buf'
Copy link
Member

Choose a reason for hiding this comment

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

$2 is the same result?

Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Signed-off-by: Masahiro Nakagawa <repeatedly@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request *Deprecated Label* Use enhancement label in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants