Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix: release watched files on completion (in read-mode) #271

Merged
merged 4 commits into from
Apr 29, 2020

Conversation

kares
Copy link
Contributor

@kares kares commented Apr 28, 2020

not doing so leads to a steady increase in watched collection's size
over time (esp. in use-cases where user is pulling in new files).

the left-over watched file entry is never to be processed again - it's being deleted anyway (assuming 'file_completed_action' => "delete") using the completion handler.

resolves #270

NOTE: please ignore the existing potential atomicity issues in the watched file collection a follow-up on this PR will deal with a WatchedFileCollection rewrite

@kares kares force-pushed the remove-watched-file-avoid-leak branch 2 times, most recently from 4bf6de5 to 9a0ca2b Compare April 28, 2020 16:27
@elasticsearch-bot elasticsearch-bot self-assigned this Apr 28, 2020
@kares kares changed the title Fix: release watched files on complete (in read-mode) Fix: release watched files on completion (in read-mode) Apr 28, 2020
@andsel andsel assigned andsel and unassigned elasticsearch-bot Apr 29, 2020
@andsel andsel self-requested a review April 29, 2020 06:48
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

In general LGTM, only a couple of questions

end
@sort_method.call
removed_files
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me why we the remove_paths returns the list of removed files. The method remove_paths is used inside the DeleteCompletedFileHandler.handle which on its own return the list of deleted files. That method is invoked by File.handle_deletable_path in an each loop and the method itself doesn't return nothing.

Copy link
Contributor Author

@kares kares Apr 29, 2020

Choose a reason for hiding this comment

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

Thanks Andrea, might have been a bit ahead of myself here ...
It's common for a (map) remove operation to return removed values.
The watched files collection is a ~ map ([String] path -> [WatchedFile] file), wanted to re-turn smt meaningful (planning to spec the return value with a collection rewrite to native).

lib/filewatch/watched_files_collection.rb Outdated Show resolved Hide resolved
@kares kares force-pushed the remove-watched-file-avoid-leak branch from 0cef200 to 59bb43e Compare April 29, 2020 11:26
@andsel andsel self-requested a review April 29, 2020 12:46
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

LGTM

@kares kares force-pushed the remove-watched-file-avoid-leak branch from 59bb43e to 309cdd8 Compare April 29, 2020 13:02
not doing so leads to a steady increase in watched collection's size
over time (esp. in use-cases where user is pulling in new files).

the left-over file is never to be processed again - it's being deleted
anyway using the completion handler.
@andsel andsel assigned kares and unassigned andsel Apr 29, 2020
@kares kares merged commit aec0be9 into logstash-plugins:master Apr 29, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

watched files seems to not get released
3 participants