logstash-plugins / logstash-input-stdin

Apache License 2.0
6 stars 16 forks source link

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

Closed colinsurprenant closed 8 years ago

colinsurprenant commented 9 years ago

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 commented 8 years ago

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

colinsurprenant commented 8 years ago

@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 commented 8 years ago

@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 https://github.com/elastic/logstash/issues/5242

ph commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@colinsurprenant Make sense.

ph commented 8 years ago

LGTM

colinsurprenant commented 8 years ago

bumped version and merged in master