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

buf_file: handle "Too many open files" error to keep buffer and metadata pair #1468

Merged
merged 3 commits into from
Feb 16, 2017
Merged

buf_file: handle "Too many open files" error to keep buffer and metadata pair #1468

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
332ee73
buf_file: handle "Too many open files" error to keep buffer and metad…
repeatedly Feb 15, 2017
c5f0d9d
Add comment for implementation detail
repeatedly Feb 16, 2017
fa29c7c
Apply review for using 'else' and add more comment for implementation
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
63 changes: 52 additions & 11 deletions lib/fluent/plugin/buffer/file_chunk.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,41 @@ def enqueued!

write_metadata(update: false) # re-write metadata w/ finalized records

file_rename(@chunk, @path, new_chunk_path, ->(new_io){ @chunk = new_io })
@path = new_chunk_path
begin
file_rename(@chunk, @path, new_chunk_path, ->(new_io) { @chunk = new_io })
rescue => e
begin
file_rename(@chunk, new_chunk_path, @path, ->(new_io) { @chunk = new_io }) if File.exist?(new_chunk_path)
rescue => re
# In this point, restore buffer state is hard because previous `file_rename` failed by resource problem.
# Retry is one possible approach but it may cause livelock under limited resources or high load environment.
# So we ignore such errors for now and log better message instead.
# "Too many open files" should be fixed by proper buffer configuration and system setting.
end
if re
Copy link
Member

Choose a reason for hiding this comment

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

It's better to use else clause to simplify code.

begin
  file_rename
rescue => re
  raise "" # using re
else
  raise "" # using e
end

raise "can't enqueue buffer file. This may causes inconsistent state: path = #{@path}, error = '#{e}', retry error = '#{re}'"
else
raise "can't enqueue buffer file: path = #{@path}, error = '#{e}'"
end
end

begin
file_rename(@meta, @meta_path, new_meta_path, ->(new_io) { @meta = new_io })
rescue => e
begin
file_rename(@chunk, new_chunk_path, @path, ->(new_io) { @chunk = new_io }) if File.exist?(new_chunk_path)
file_rename(@meta, new_meta_path, @meta_path, ->(new_io) { @meta = new_io }) if File.exist?(new_meta_path)
rescue => re
# See above
end
if re
Copy link
Member

Choose a reason for hiding this comment

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

ditto

raise "can't enqueue buffer metadata. This may causes inconsistent state: path = #{@meta_path}, error = '#{e}', retry error = '#{re}'"
else
raise "can't enqueue buffer metadata: path = #{@meta_path}, error = '#{e}'"
end
end

file_rename(@meta, @meta_path, new_meta_path, ->(new_io){ @meta = new_io })
@path = new_chunk_path
@meta_path = new_meta_path

super
Expand Down Expand Up @@ -242,14 +273,24 @@ def file_rename(file, old_path, new_path, callback=nil)
def create_new_chunk(path, perm)
@path = self.class.generate_stage_chunk_path(path, @unique_id)
@meta_path = @path + '.meta'
@chunk = File.open(@path, 'wb+', perm)
@chunk.set_encoding(Encoding::ASCII_8BIT)
@chunk.sync = true
@chunk.binmode
@meta = File.open(@meta_path, 'wb', perm)
@meta.set_encoding(Encoding::ASCII_8BIT)
@meta.sync = true
@meta.binmode
begin
@chunk = File.open(@path, 'wb+', perm)
@chunk.set_encoding(Encoding::ASCII_8BIT)
@chunk.sync = true
@chunk.binmode
rescue => e
raise BufferOverflowError, "can't create buffer file for #{path}. Stop creating buffer files: error = #{e}"
Copy link
Member

Choose a reason for hiding this comment

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

Does this statement expect "too many open files" or exceptions like that?
Please add a comment about expected exception types - it seems not the direct "buffer overflow" error.

end
begin
@meta = File.open(@meta_path, 'wb', perm)
@meta.set_encoding(Encoding::ASCII_8BIT)
@meta.sync = true
@meta.binmode
rescue => e
@chunk.close
Copy link
Member

Choose a reason for hiding this comment

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

this requires rescue nil.

File.unlink(@path)
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

raise BufferOverflowError, "can't create buffer metadata for #{path}. Stop creating buffer files: error = #{e}"
Copy link
Member

Choose a reason for hiding this comment

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

Same comment required above.

end

@state = :unstaged
@bytesize = 0
Expand Down