search

elastic / logstash

Logstash - transport and process your logs, events, or other data
https://www.elastic.co/products/logstash
Other
14.18k stars 3.5k forks source link

Adapt plugins to new Plugin API introduced by #3210 #3813

Closed jsvd closed 8 years ago

jsvd commented 9 years ago

Issue #3210 and PR #3812 #3895 change the Plugin API to:

  1. add stop (public), stop? (public) to LogStash::Inputs::Base
  2. rename teardown to close
  3. remove shutdown, finished. finished?, running?, terminating? from LogStash::Plugin

    Refactor work needed on the plugins

    Input plugins

Input plugins are now shutdown by an external call to plugin.stop instead of catching LogStash::ShutdownSignal exception. Unless overridden, stop will simply makes stop? return true, thus allowing run to poll this and return after seeing the change.

In some plugins extra work must be done in stop to instruct run that it's time to return. For example, in the logstash-input-udp it's necessary to call @socket.close to make the blocking read on the socket raise an exception, thus breaking out the loop. So, different input plugins will require different stop strategies.

Refactoring an input plugin involves:

Then for testing you can use the shared example provided in https://github.com/elastic/logstash-devutils/pull/32 in the following manner:

describe LogStash::Inputs::Http do
  let(:port) { rand(5000) + 1025 }
  it_behaves_like "an interruptible input plugin" do
    let(:config) { { "port" => port } }
  end
end
Testing the refactor

1) clone logstash, switch to PR 3895 2) clone logstash-devutils, switch to PR 32 3) clone plugin repository, switch to refactor PR (check above) 4) edit Gemfile to look like:

source 'https://rubygems.org'
gemspec
gem "logstash-core", :path => "/path/to/git/project/logstash"
gem "logstash-devutils", :path => "/path/to/git/project/logstash-devutils"

5) run bundle install 6) run bundle exec rspec

Filters and Outputs

Both will still shutdown using the ShutdownEvent sent by the pipeline so no major changes necessary. The work in these plugins is:

Work To Do

1. Input plugins

1.1 Default Input Plugins

Nothing to do.

2. Remove finished

Nothing to do.

4. Remove running?

Nothing to do.

5. Remove terminating?

Nothing to do.

6. Rename teardown to close

Issue https://github.com/elastic/logstash/issues/3952 tracks this

7. Other tasks

ph commented 9 years ago

Awesome @jsvd!

suyograo commented 9 years ago

@jsvd thanks for the detailed analysis! I added 2 more points at the end

jsvd commented 9 years ago

For all interested in testing this:

1) clone logstash, switch to PR 3812 (now 3895) 2) clone logstash-devutils, switch to PR 32 3) clone plugin repository, switch to refactor PR (check above) 4) edit Gemfile to look like:

source 'https://rubygems.org'
gemspec
gem "logstash-core", :path => "/path/to/git/project/logstash"
gem "logstash-devutils", :path => "/path/to/git/project/logstash-devutils"

5) run bundle install 6) run bundle exec rspec

purbon commented 9 years ago

@jsvd work on a few inputs here, waiting to get more feedback from you before moving forward with more. Open question for me here is how deep to go on the "cleanup" ? :smile_cat:

colinsurprenant commented 9 years ago

before moving forward too deep (in particular for close/teardown and stop? we need to settle on #3810 and #3812 (now #3895)

@purbon we should avoid mixing cleanup & refactor work. one or the other but not both in the same PR, start with refactor since this is what is required here. after we can add/merge cleanups.

colinsurprenant commented 9 years ago

please note that some input plugins might need special handling/testing for when in blocked IO state to see if they can be unblocked from the pipeline thread form the stop method.

jsvd commented 9 years ago

@colinsurprenant indeed. the one exception I'm not worried about is the logstash-input-stdin, since it's equally non interruptible with both the new and old apis. Even with the PR you created to use the java interopt to extract the channel and make it interruptible, it's a non breaking change so it doesn't need to be rushed, IMO.

colinsurprenant commented 9 years ago

@jsvd +1

wiibaa commented 9 years ago

@colinsurprenant sorry to bump in, but as you are planning this huge refactoring, could you consider adding the small refactor described in #2447 about abstracting into base how to send an event to the filterworker(s) instead of exposing the queue in the run method. IMHO this could be the baseline for future enhancement on how inputs would be submitting events to the pipeline and allow to centralize ideas about detection/handling of back-pressure, batching of event, prioritization, dispatching, queue persistence etc etc. Thanks for reading!

colinsurprenant commented 9 years ago

@wiibaa thanks for the heads up - I will followup on #2447.

colinsurprenant commented 9 years ago

@jsvd @ph @jordansissel could use a review(s) of jordansissel/ruby-stud#27 to move forward with all plugins using Stud.interval

andrewvc commented 9 years ago

Is there a reason that 'stop' and 'close' have different names? I think this will be very confusing to future plugin authors. It's already confusing to me now. Ideally they would both also make stop? work correctly (some outputs sleep).

talevy commented 9 years ago

@andrewvc

I think teardown(close) was originally intended for cleaning up, not instructing a plugin to effectively "stop" execution. one can be viewed as an "instruction" given to a running plugin. while close is an "instruction" given on a plugin that has been "stopped" (but might actually be running in the background via threads and open file handles).

I think it would be ok to rename stop to close for inputs. The only gotcha is that inputs that wish to override this and implement extra teardown actions will have to remember to call super. To avoid this, we can have the pipeline call another function that is not a part of the public api that will run the @stop_called.make_true stuff.

what do you think?

andrewvc commented 9 years ago

@talevy I'm not clear on why they need to call super? Presuming that they don't ever check stop? what would the issue be?

talevy commented 9 years ago

I don't like the fact that people who wish to use #stop as a place to teardown have to know about the guts of state management around @stop_called. It is true, if stop? is never used, this is a non-issue. I just don't think it is very nice to have to know whether this is a non-issue or an issue.

andrewvc commented 9 years ago

@talevy I misread your original comment. I do think we should have another function called externally that makes stop? return true. I think we should have a __step_stop? or the like function that does this that no one ever overrides.

talevy commented 9 years ago

disagree on the naming :) but yeah, that is what I meant!

andrewvc commented 9 years ago

@talevy I meant __set_stop(true).

jsvd commented 8 years ago

all default plugins now have been refactored to address the new api. tracking non-default plugins in https://github.com/elastic/logstash/issues/3963