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

use jruby-stdin-channel if possible for an interruptible stdin #3

Merged

Conversation

colinsurprenant
Copy link
Contributor

solves to uninterruptible stdin when running on Java 8.
if running on Java 8, doing ^C will correctly terminate logstash unlike before where you had to type an extra character to unblock stdin.
related to elastic/logstash#1769

I only tested on OSX with Java 7 and 8.

I did not measure any noticeable performance difference using jruby-stdin-channel or not.

@ph
Copy link
Contributor

ph commented Dec 3, 2015

@colinsurprenant Can you rebase it off master? The specific stdin channel handling look good but I haven't tested it.

@colinsurprenant
Copy link
Contributor Author

@ph rebased and bumped version. since specs are rather basic we should either have integration tests for both Java 7 + Java 8 or do manual tests on both JVM.

@ph ph self-assigned this May 4, 2016
@ph
Copy link
Contributor

ph commented May 4, 2016

@colinsurprenant I will do a small manual testing for that. This only work on java 8?

We can probably drop the compatibility for java 7 in this PR and only merge it to the master branch?
Because we will bail out if the user try to run logstash under java 7 see elastic/logstash#5242

@ph
Copy link
Contributor

ph commented May 4, 2016

LGTM

Tested with core with JDK7 and JDK8

^CSIGINT received. Shutting down the agent. {:level=>:warn}
stopping pipeline {:id=>"main", :level=>:warn}
^CSIGINT received. Terminating immediately.. {:level=>:fatal}
^CSIGINT received. Terminating immediately.. {:level=>:fatal}
Received shutdown signal, but pipeline is still waiting for in-flight events
to be processed. Sending another ^C will force quit Logstash, but this may cause
data loss. {:level=>:warn}
^Z
[2]  + 12237 suspended  bin/logstash -e 'input { stdin {} } output { stdout {}}'
JDK8
[2]  + 12237 killed     bin/logstash -e 'input { stdin {} } output { stdout {}}'
~/e/logstash git:master ❯❯❯ jdk8                                                                                                                                                                                                                                                                                        ✱ ◼
~/e/logstash git:master ❯❯❯ bin/logstash -e 'input { stdin {} } output { stdout {}}'                                                                                                                                                                                                                                    ✱ ◼
--- jar coordinate com.fasterxml.jackson.core:jackson-annotations already loaded with version 2.7.1 - omit version 2.7.0
--- jar coordinate com.fasterxml.jackson.core:jackson-databind already loaded with version 2.7.1 - omit version 2.7.1-1
Settings: Default pipeline workers: 4
Pipeline main started
^CSIGINT received. Shutting down the agent. {:level=>:warn}
stopping pipeline {:id=>"main", :level=>:warn}
Pipeline main has been shutdown
~/e/logstash git:master ❯❯❯


@colinsurprenant
Copy link
Contributor Author

colinsurprenant commented May 6, 2016

@ph we should keep the normal stdin fallback because the jruby-stdin-channel gem uses a pretty implementation specific way of retrieving the nio channel from stdin and this could fail on future versions or alternate JVMs for example and if it fails we revert back to using the normal stdin...

@ph
Copy link
Contributor

ph commented May 6, 2016

@colinsurprenant Make sense.

@ph
Copy link
Contributor

ph commented May 9, 2016

LGTM

bumped to v2.1.0
@colinsurprenant colinsurprenant merged commit d33b054 into logstash-plugins:master May 25, 2016
@colinsurprenant
Copy link
Contributor Author

bumped version and merged in master

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.

3 participants