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

in_tail: avoid unnecessary open()s #788

Merged
merged 1 commit into from
Feb 12, 2016

Conversation

dwoo4dwoo
Copy link

@dwoo4dwoo
Copy link
Author

The Travis CI build failures seem unrelated to my change ... (UdpInputTest EADDRINUSE)

@repeatedly repeatedly self-assigned this Jan 31, 2016
@repeatedly
Copy link
Member

Possible file rotation cases:

  • File is rotated before stat = FileWrapper.stat(@path)

No problem.

  • File is rotated ater stat = FileWrapper.stat(@path)
    • if @inode != inode || fsize < @fsize is false: No problem. Next trigger should detect file rotation.
    • if @inode != inode || fsize < @fsize is true: It seems no problem. on_rotate also checks stat info with previous file. So this patch should not effect existing behaviour.

Does anyone have any concern for this patch?

@repeatedly
Copy link
Member

If there are no problem, I will merge this patch today.

@@ -593,13 +592,11 @@ def on_notify
begin
if @inode != inode || fsize < @fsize
# rotated or truncated
io = FileWrapper.open(@path)
Copy link
Member

Choose a reason for hiding this comment

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

When Errno::ENOENT occurs on FileWrapper.stat(@path), it seems Errno::ENOENT occurs again at here. Do not you need to handle it?

Copy link
Member

Choose a reason for hiding this comment

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

So adding rescue nil is better?
io = FileWrapper.open(@path) rescue nil
or

begin
  io = FileWrapper.open(@path)
rescue Errno::ENOENT
end

Copy link
Author

Choose a reason for hiding this comment

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

@sonots : good catch - thanks!
@repeatedly : I'll update the pull request with your suggestion

@repeatedly
Copy link
Member

@sonots Could you check again?

@@ -593,13 +592,11 @@ def on_notify
begin
if @inode != inode || fsize < @fsize
# rotated or truncated
io = FileWrapper.open(@path) rescue nil
Copy link
Member

Choose a reason for hiding this comment

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

Could you tell me when this io object will be closed? Or, not necessary to close? Why?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why there was

 io = nil

before. Maybe, we need to keep previous io object as @io like @iNode and @fsize to ensure to close it.

Copy link
Member

Choose a reason for hiding this comment

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

You should ignore only Errno::ENOENT as repeatedly wrote.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @sonots ,

Could you tell me when this io object will be closed?

The file descriptor is consumed here:

https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/in_tail.rb#L384

The first time through ... an io_handler is created here:

https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/in_tail.rb#L416

The file descriptor remains open until the next file rotation, removal, or fluentd shutdown. The logic to deal with some of these situations begins here:

https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/in_tail.rb#L420

Other cases are handled here:

https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/in_tail.rb#L188

For example, shutdown calls stop_watchers().

You should ignore only Errno::ENOENT as repeatedly wrote

Sorry, I thought @repeatedly was giving me a choice, and I picked the first option. I'll update the pull request using the second option. I'll also rebase against master.

David

@sonots
Copy link
Member

sonots commented Feb 10, 2016

LGTM

repeatedly added a commit that referenced this pull request Feb 12, 2016
in_tail: avoid unnecessary open()s
@repeatedly repeatedly merged commit 64c5d0c into fluent:master Feb 12, 2016
@repeatedly
Copy link
Member

Thanks for the patch!

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.

3 participants