-
Notifications
You must be signed in to change notification settings - Fork 52
Fix behaviour when the destination file is deleted #21
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
require "stud/temporary" | ||
require "tempfile" | ||
require "uri" | ||
require "fileutils" | ||
|
||
describe LogStash::Outputs::File do | ||
describe "ship lots of events to a file" do | ||
|
@@ -106,6 +107,58 @@ | |
end | ||
|
||
describe "receiving events" do | ||
|
||
context "when the output file is deleted" do | ||
|
||
let(:temp_file) { Tempfile.new('logstash-spec-output-file_deleted') } | ||
|
||
let(:config) do | ||
{ "path" => temp_file.path, "flush_interval" => 0 } | ||
end | ||
|
||
it "should recreate the required file if deleted" do | ||
output = LogStash::Outputs::File.new(config) | ||
output.register | ||
|
||
10.times do |i| | ||
event = LogStash::Event.new("event_id" => i) | ||
output.receive(event) | ||
end | ||
FileUtils.rm(temp_file) | ||
10.times do |i| | ||
event = LogStash::Event.new("event_id" => i+10) | ||
output.receive(event) | ||
end | ||
expect(FileTest.size(temp_file.path)).to be > 0 | ||
end | ||
|
||
context "when appending to the error log" do | ||
|
||
let(:config) do | ||
{ "path" => temp_file.path, "flush_interval" => 0, "create_if_deleted" => false } | ||
end | ||
|
||
it "should append the events to the filename_failure location" do | ||
output = LogStash::Outputs::File.new(config) | ||
output.register | ||
|
||
10.times do |i| | ||
event = LogStash::Event.new("event_id" => i) | ||
output.receive(event) | ||
end | ||
FileUtils.rm(temp_file) | ||
10.times do |i| | ||
event = LogStash::Event.new("event_id" => i+10) | ||
output.receive(event) | ||
end | ||
expect(FileTest.exist?(temp_file.path)).to be_falsey | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternate way to write this, not sure if you like it:
(I think in this case, my alternative is more confusing, so let's ignore it, hehe) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think your solution is better in terms of being syntactically correct and probably giving a better error message. But is also more confusing to read, so I would stick to the current one. |
||
expect(FileTest.size(output.failure_path)).to be > 0 | ||
end | ||
|
||
end | ||
|
||
end | ||
|
||
context "when using an interpolated path" do | ||
context "when trying to write outside the files root directory" do | ||
let(:bad_event) do | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This introduces up to 2 File.stat calls per event sent to this output where previously there were zero File.stat calls per event.
I wonder about the performance implications of this.
Maybe we should only bother checking if the file is deleted if
@create_if_deleted
is true, that way the performance penalty is isolated to those wishing to use this 'create-if-deleted' behavior?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a fair concern, however I added another "feature" here, if the file is deleted but
@create_if_deleted
is false, it appends the data to the failure file, like this there is no chance stuff is "lost". What do you think?