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

jsvd · 2015-08-28T10:22:35Z

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

add stop (public), stop? (public) to LogStash::Inputs::Base
rename teardown to close
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:

removing rescue of ShutdownSignal exception
understand the nature of the run loop: is it a while? a consumer.subscribe {|event| }? is there a blocking operation on a socket/fd?
use stop? and/or override stop to make run return
put any other cleanup/bookkeeping tasks in close (currently done in teardown)
remove any calls to shutdown, finished, finished?, running? or terminating?

Then for testing you can use the shared example provided in elastic/logstash-devutils#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

clone logstash, switch to PR 3895
clone logstash-devutils, switch to PR 32
clone plugin repository, switch to refactor PR (check above)
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"

run bundle install
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:

to remove calls that are no longer in the pipeline <-> plugin contract: shutdown, finished, finished?, running? or terminating?
rename teardown to close

Work To Do

1. Input plugins

1.1 Default Input Plugins

1. Remove `shutdown`

Nothing to do.

2. Remove `finished`

logstash-output-file: PLEASE REVIEW PR remove useless call to finished logstash-plugins/logstash-output-file#16
logstash-output-kafka: PLEASE REVIEW PR update to new shutdown api logstash-plugins/logstash-output-kafka#28
logstash-output-lumberjack: PLEASE REVIEW PR remove useless call to finished logstash-plugins/logstash-output-lumberjack#10
logstash-output-pipe: PLEASE REVIEW remove finished from teardown logstash-plugins/logstash-output-pipe#3
logstash-output-rabbitmq: Please Review Massive Refactor! logstash-plugins/logstash-output-rabbitmq#23
logstash-output-s3: PLEASE REVIEW Remove finished from #teardown logstash-plugins/logstash-output-s3#30
logstash-output-sqs: PLEASE REVIEW Remove finished and we don't need to explicitely set the @sqs to nil, the gc will take care of it. logstash-plugins/logstash-output-sqs#8
logstash-output-stdout: PLEASE REVIEW PR remove useless call to finished logstash-plugins/logstash-output-stdout#4
logstash-output-udp: PLEASE REVIEW PR remove finished logstash-plugins/logstash-output-udp#4

3. Remove `finished?`

Nothing to do.

4. Remove `running?`

Nothing to do.

5. Remove `terminating?`

Nothing to do.

6. Rename `teardown` to `close`

Issue #3952 tracks this

7. Other tasks

Improve the logstash-input-example documentation to reflect the new API
Add documentation in elastic.co reference about new API signatures
Blog about API changes
Fix plugins not included by default in logstash releases - Track shutdown refactor for other plugins not packaged with logstash #3963

The text was updated successfully, but these errors were encountered:

ph · 2015-08-28T12:47:59Z

Awesome @jsvd!

suyograo · 2015-08-28T15:40:00Z

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

jsvd · 2015-08-31T16:32:53Z

For all interested in testing this:

clone logstash, switch to PR 3812 (now 3895)
clone logstash-devutils, switch to PR 32
clone plugin repository, switch to refactor PR (check above)
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"

run bundle install
run bundle exec rspec

purbon · 2015-09-01T13:29:33Z

@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" ? 😸

Ref elastic/logstash#3813

colinsurprenant · 2015-09-04T20:59:44Z

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 · 2015-09-04T21:01:59Z

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 · 2015-09-07T15:05:18Z

@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 · 2015-09-08T15:34:50Z

@jsvd +1

wiibaa · 2015-09-11T04:41:46Z

@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 · 2015-09-12T22:38:56Z

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

colinsurprenant · 2015-09-14T02:37:06Z

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

andrewvc · 2015-09-17T21:20:33Z

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 · 2015-09-17T21:37:09Z

@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 · 2015-09-17T21:39:03Z

@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 · 2015-09-17T21:42:46Z

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 · 2015-09-17T21:52:43Z

@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 · 2015-09-17T21:53:32Z

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

andrewvc · 2015-09-17T21:57:07Z

@talevy I meant __set_stop(true).

Ref elastic/logstash#3813

Ref elastic/logstash#3813 Fixes #3

jsvd · 2015-10-12T13:39:33Z

all default plugins now have been refactored to address the new api.
tracking non-default plugins in #3963

jsvd mentioned this issue Aug 28, 2015

[Design] plugins shutdown sequence refactor proposal #3210

Closed

suyograo added meta breaking change resiliency labels Aug 28, 2015

suyograo added the v2.0.0 label Sep 1, 2015

ph added a commit to ph/logstash-output-s3 that referenced this issue Sep 1, 2015

Remove finished from #teardown

f478916

Ref elastic/logstash#3813

ph mentioned this issue Sep 1, 2015

Remove finished from #teardown logstash-plugins/logstash-output-s3#30

Closed

ph added a commit to ph/logstash-output-redmine that referenced this issue Sep 1, 2015

Remove finished

fb47e74

Ref elastic/logstash#3813

This was referenced Sep 1, 2015

Remove finished logstash-plugins/logstash-output-redmine#3

Closed

remove finished from teardown logstash-plugins/logstash-output-pipe#3

Closed

jsvd added shutdown semantics and removed resiliency labels Sep 9, 2015

talevy mentioned this issue Sep 15, 2015

Update Plugin Shutdown to use new close method logstash-plugins/logstash-output-kafka#27

Closed

ph added a commit to logstash-plugins/logstash-output-s3 that referenced this issue Sep 21, 2015

Remove finished from #teardown

3222547

Ref elastic/logstash#3813

ph added a commit to logstash-plugins/logstash-output-redmine that referenced this issue Sep 21, 2015

Remove finished

58a2cab

Ref elastic/logstash#3813 Fixes #3

jsvd mentioned this issue Sep 24, 2015

Track shutdown refactor for other plugins not packaged with logstash #3963

Open

53 tasks

jsvd closed this as completed Oct 12, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jsvd commented Aug 28, 2015

ph commented Aug 28, 2015

suyograo commented Aug 28, 2015

jsvd commented Aug 31, 2015

purbon commented Sep 1, 2015

colinsurprenant commented Sep 4, 2015

colinsurprenant commented Sep 4, 2015

jsvd commented Sep 7, 2015

colinsurprenant commented Sep 8, 2015

wiibaa commented Sep 11, 2015

colinsurprenant commented Sep 12, 2015

colinsurprenant commented Sep 14, 2015

andrewvc commented Sep 17, 2015

talevy commented Sep 17, 2015

andrewvc commented Sep 17, 2015

talevy commented Sep 17, 2015

andrewvc commented Sep 17, 2015

talevy commented Sep 17, 2015

andrewvc commented Sep 17, 2015

jsvd commented Oct 12, 2015

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

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

Comments

jsvd commented Aug 28, 2015

Refactor work needed on the plugins

Input plugins

Testing the refactor

Filters and Outputs

Work To Do

1. Input plugins

1.1 Default Input Plugins

1. Remove shutdown

2. Remove finished

3. Remove finished?

4. Remove running?

5. Remove terminating?

6. Rename teardown to close

7. Other tasks

ph commented Aug 28, 2015

suyograo commented Aug 28, 2015

jsvd commented Aug 31, 2015

purbon commented Sep 1, 2015

colinsurprenant commented Sep 4, 2015

colinsurprenant commented Sep 4, 2015

jsvd commented Sep 7, 2015

colinsurprenant commented Sep 8, 2015

wiibaa commented Sep 11, 2015

colinsurprenant commented Sep 12, 2015

colinsurprenant commented Sep 14, 2015

andrewvc commented Sep 17, 2015

talevy commented Sep 17, 2015

andrewvc commented Sep 17, 2015

talevy commented Sep 17, 2015

andrewvc commented Sep 17, 2015

talevy commented Sep 17, 2015

andrewvc commented Sep 17, 2015

jsvd commented Oct 12, 2015

1. Remove `shutdown`

2. Remove `finished`

3. Remove `finished?`

4. Remove `running?`

5. Remove `terminating?`

6. Rename `teardown` to `close`