LOGSTASH-1799 Don't always restart pipe commands. #942
Conversation
Allow the pipe to be configurable about what should happen when the command exists. Valid things todo are restart, restart on errors and never restart.
| +1 |
| Would you mind doing Step 2 of the CONTRIBUTING doc? |
lib/logstash/inputs/pipe.rb Outdated
There was a problem hiding this comment.
You can actually enforce enumerated validation here:
:validate => [ "always", "error", "never" ] | I've signed the CLA . Thanks for the tip about validation and I've improved the variable name. |
lib/logstash/inputs/pipe.rb Outdated
There was a problem hiding this comment.
I think this is meant to be 'relaunch = ...' right? I can fix this post-merge
| I've fixed the old use of |
| Is there anything else that needs doing to get this merged? |
| @colinsurprenant any chance to add this "little" one to 1.4.2 schedule, it could be considered a bugfix because #619 broke the use case where people used the pipe input as a backfill single execution as discussed in the comments of https://logstash.jira.com/browse/LOGSTASH-966. |
| thanks, overall looks good. my only problem is that we don't have specs for this (there's none for the pipe input) so there's no way to validate this without running it. ideally if you could add specs, that would be great, otherwise I'll put it in the pipeline for the next release but will require us to add specs before including it for the release. let me know if you'd like to also work on the specs, we could certainly help you for this. I am wondering why this 10 seconds sleep @jordansissel ? seems a very long delay? |
| @colinsurprenant I have made a PR with an initial spec here #1385 against the current master. |
| I merged #1385 |
There was a problem hiding this comment.
I don't really like the
begin ... end while ...construct. in this case since the logic is pretty simple, I'd put back the loop do and replace both relaunch = with a break.
break unless @restart == "always" break unless @restart == "error"and put back the sleep(10) at the end of the loop
There was a problem hiding this comment.
and while at it, why don't we make the sleep time configurable and default to 10 seconds?
| I opened this as a bugfix because of the original change introduced in: https://logstash.jira.com/browse/LOGSTASH-966 |
| @buckett your point is already clear, I simply restarted the discussion to get this PR merged. The question now is if you have time to look to Colin comments in the code and help get this merged hopefully soon |
| @wiibaa I will, but I don't have time in the next few weeks, if someone else is able to address Colin's points feel free to take over. And thanks for your help in getting eyes on this Wibba and sorting out the specs. |
| @jordansissel @colinsurprenant can you please close this one in favor of #1415 |
| Can one of the admins verify this patch? |
Allow the pipe to be configurable about what should happen when the command exists. Valid things todo are restart, restart on errors and never restart.