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

Jump to bottom

Ability to terminate pipeline when EOF reached #240

Closed
wants to merge 1 commit into from
Closed

Ability to terminate pipeline when EOF reached #240

wants to merge 1 commit into from

Conversation

BigYellowHammer
Copy link
Contributor

Adding ability to terminate pipeline when EOF reached (#212)
Change is backwards compatible. When flag is set to 'false' - no change to code execution.

Changes:
* Adding exit_after_read flag to config
* Disable active discovery when exit_after_read set to true
* Remove file from watched_file_collection when EOF reached and flag set to true
* Adding condition to exit when file collection is empty and flag set to true
* Fixing WatchedFilesCollection delete method bug (unable to delete multiple files with 1 method call)
* Adding exception when exit_after_read it set to true in tail mode

Fixes #212

@BigYellowHammer
Copy link
Contributor Author

Can anybody at least take a look there and let me know if the approach is correct?

@jbwl
Copy link

jbwl commented Jul 12, 2019

Would be nice if this functionality could be merged soon.

@BigYellowHammer
Copy link
Contributor Author

Any chance to merge this change? Or this plugin is no longer in active development?

@gregorysl
Copy link

@karenzone @ddaanet

karenzone
karenzone reviewed Jan 17, 2020
View reviewed changes
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

@BigYellowHammer, thanks for this contribution to the input-file plugin and the docs! I have reviewed the docs. Nice job describing the new option and its use! One request: Please consider moving the new option to alpha order rather than operational order. That standard makes it easier for users to track down options.

@karenzone
Copy link
Contributor

@gregorysl Thanks for the ping. I have reviewed the docs, and requested a tech review for the plugin.
@BigYellowHammer Thanks again for the contribution, and for a great job on the docs!

@BigYellowHammer
Copy link
Contributor Author

BigYellowHammer commented Jan 17, 2020 •
edited
Loading

@karenzone Attributes ordering is fixed.

@andsel
Copy link
Contributor

andsel commented Jan 21, 2020

Hi @BigYellowHammer it's probably better to separate the bugfix of WatchedFilesCollection in a separate PR.

@BigYellowHammer
Copy link
Contributor Author

Hi @andsel WatchedFilesCollection fix is essential for my exit_after_read flag to work

@andsel
Copy link
Contributor

andsel commented Jan 21, 2020

@BigYellowHammer we could work before on the bugfix, and then progress with this one after a rebase of the code

@BigYellowHammer
Copy link
Contributor Author

@andsel ok will try to prepare new PR this week

andsel
andsel reviewed Jan 21, 2020
View reviewed changes
lib/filewatch/watch.rb Outdated Show resolved Hide resolved
karenzone
karenzone reviewed Jan 21, 2020
View reviewed changes
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

@BigYellowHammer Docs LGTM. Builds cleanly and looks great! Thanks for your contribution.
If you need to make additional docs changes and would like for me to take another look, please ping me.

@BigYellowHammer BigYellowHammer mentioned this pull request Jan 24, 2020
Closed
andsel
andsel reviewed Feb 18, 2020
View reviewed changes
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.

Some notes after running it with a pipeline and some files

lib/filewatch/read_mode/processor.rb
# handlers take care of closing and unwatching
end
end

def common_detach_when_allread(watched_file)
watched_file.unwatch
watched_file.listener.reading_completed
Copy link
Contributor

Choose a reason for hiding this comment

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

@BigYellowHammer this method is undefined in https://github.com/BigYellowHammer/logstash-input-file/blob/feature/terminate-when-EOF/lib/logstash/inputs/file_listener.rb

Copy link
Contributor

Choose a reason for hiding this comment

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

Method common_detach_when_allread It's like the method common_deleted_reaction, I think we could simply put call common_deleted_reaction(watched_file, "Completed") and avoid common_detach_when_allread. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed it, just added the missed callback reading_completed method to LogStash::Inputs::FileListener

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to keep my functionality separate from the main code. Plus common_deleted_reaction is more like error handling method when the file gets deleted IMO. In order to merge those methods I will have to put if inside common_deleted_reaction base on the action and this action is string so it might be error prone to spelling mistakes. So I would rather keep them separate or change string to enum if we really want to merge it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@BigYellowHammer I agree with you, the commit 82e58b0 has fixed that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andsel build failed? Do you think it's intermittent. Can we retrigger?

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw, and restarting make it always fails. I try to reproduce locally

Copy link
Contributor

Choose a reason for hiding this comment

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

@BigYellowHammer the error happens also on master, so it's not related to this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andsel do we even need this "an interruptible input plugin" shared example? is it used by some libraries above? because I don't see any direct usage in logstash-input-file spec folder

@andsel andsel force-pushed the feature/terminate-when-EOF branch 2 times, most recently from 82e58b0 to 6452aa3 Compare February 19, 2020 12:16
@elasticsearch-bot
Copy link

Andrea Selva merged this into the following branches!

Branch Commits
master cc2fbc0

@andsel
Copy link
Contributor

andsel commented Feb 20, 2020 •
edited
Loading

@BigYellowHammer many thanks for your contribution with this PR

@BigYellowHammer BigYellowHammer deleted the feature/terminate-when-EOF branch February 20, 2020 11:00
@andsel andsel mentioned this pull request Apr 1, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andsel andsel andsel left review comments

@karenzone karenzone karenzone left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Option to terminate pipeline when EOF is reached
6 participants
@BigYellowHammer @jbwl @gregorysl @karenzone @andsel @elasticsearch-bot