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.

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

Make usage of the new stop methodology #2

Closed
wants to merge 1 commit into from
Closed

Make usage of the new stop methodology #2

wants to merge 1 commit into from

Conversation

purbon
Copy link

@purbon purbon commented Sep 1, 2015

Implements stop to return from run according to elastic/logstash#3210

Depends on elastic/logstash-devutils#32 and elastic/logstash#3895

@jsvd
Copy link
Member

jsvd commented Sep 18, 2015

EDITED: nevermind, tests pass
are you considering using the stoppable_sleep and etc or is this a final PR?

@purbon
Copy link
Author

purbon commented Sep 18, 2015

@jsvd should update to stoppable_sleep yes.

@purbon
Copy link
Author

purbon commented Sep 18, 2015

queue << event
end
out.close
rescue Exception => e
Copy link
Member

Choose a reason for hiding this comment

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

any reason to catch Exception instead of StandardError? (just doing rescue => e)

@jsvd
Copy link
Member

jsvd commented Sep 21, 2015

I'd prefer not to have the refactoring done in e8bf7d8, but otherwise LGTM

[edit] please squash before merge

…roject

Make the command interrumpible by closing the stream, also introduced
some small refactoring to pull things into their own methods and out of
the main run.

make usage of Stud.stoppable_stop to actually stop even if the thread is sleeping
@elasticsearch-bot
Copy link

Merged sucessfully into master!

@purbon purbon closed this in 48ab605 Sep 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants