Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Port two Harvester tests of log input to filestream in Golang #24190
Port two Harvester tests of log input to filestream in Golang #24190
Changes from 3 commits
15d61e4
540f8b7
373f1b0
18203b6
9ff606c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Really? I wonder if the issue is due to use trying to write to the "original" path after the rotation.
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.
I got this error when I ran
os.Rename
:The process cannot access the file because it is being used by another process.
I think no because the test tried to rename the file after it had written lines to the file. It rather fails because the
filestream
input keeps the file open as it is waiting for new events to show up. To test my theory I closed the file on EOF, afterwards, I was able to rename the file on Windows as well.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.
When reopening restarting the input, is that detected as a rename? Can we create a test to validate the expected 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.
If someone is able to rename a file on Windows while Filebeat is keeping it open, it is detected by the checker goroutine of
logReader
. So the reader is going to behave correctly.What is the expected behaviour? If the input is restarted, it does not make sense to check if a file was renamed because
close_renamed
applies to opened files that are renamed during Filebeat running aHarvester
for.When the input is restarted, it finds the state in the registry for the renamed file, unless
path
file_identity
strategy is selected, it will not start a newHarvester
for the file because it is still the same file it encountered during the previous run.These test cases are not special, we either already have tests for it or going to have them when all tests of
log
input is ported to usefilestream
.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.
I still would expect that the meta-data in the registry are updated... meaning that the prospector detects the file was renamed. Maybe it is rather worth an unit test for the prospector
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.
OK, I will address this in a separate PR.