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.

Sign up for GitHub

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

Jump to bottom

Refactor the shutdown #19

Closed
wants to merge 1 commit into from
Closed

Refactor the shutdown #19

wants to merge 1 commit into from

Conversation

ph
Copy link
Contributor

@ph ph commented Sep 17, 2015

This PR introduces the changes needed to follow the new shutdown
semantic, it also remove the gelf as a runtime dependencies and move it
as a development dependency since its only used in the tests.

Fixes #17

@jsvd jsvd mentioned this pull request Sep 17, 2015
Closed
43 tasks
jsvd
jsvd reviewed Sep 18, 2015
View reviewed changes
spec/inputs/gelf_spec.rb
Thread.abort_on_exception = true
describe LogStash::Inputs::Gelf do
context "when interrupting the plugin" do
let(:port) { "12333" }
Copy link
Member

Choose a reason for hiding this comment

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

unless we randomize this we'll probably have issues when jenkins tests the multiple jvms in parallel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, changing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and wth I used a string here.

Copy link

Choose a reason for hiding this comment

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

yay! doing rand(1024...65532) I guess would be the most you need :P

@jsvd
Copy link
Member

jsvd commented Sep 18, 2015

there might be some race condition happening here, I get the following every 1 in 5 or 6

logstash-input-gelf (git)-[pr/19] % bundle exec rspec 
Using Accessor#strict_set for specs
Run options: exclude {:redis=>true, :socket=>true, :performance=>true, :couchdb=>true, :elasticsearch=>true, :elasticsearch_secure=>true, :export_cypher=>true, :integration=>true, :windows=>true}
.F

Failures:

  1) LogStash::Inputs::Gelf when interrupting the plugin behaves like an interruptible input plugin #stop returns from run
     Failure/Error: subject.stop
     IOError:
       No such file or directory
     Shared Example Group: "an interruptible input plugin" called from ./spec/inputs/gelf_spec.rb:18
     # ./lib/logstash/inputs/gelf.rb:73:in `stop'
     # /Users/joaoduarte/projects/logstash-devutils/lib/logstash/devutils/rspec/shared_examples.rb:16:in `(root)'
     # /Users/joaoduarte/.rvm/gems/jruby-1.7.19/gems/rspec-wait-0.0.7/lib/rspec/wait.rb:46:in `(root)'

Finished in 1 second (files took 2.66 seconds to load)
2 examples, 1 failure

Failed examples:

rspec /Users/joaoduarte/projects/logstash-devutils/lib/logstash/devutils/rspec/shared_examples.rb:9 # LogStash::Inputs::Gelf when interrupting the plugin behaves like an interruptible input plugin #stop returns from run

Randomized with seed 22953

@ph
Copy link
Contributor Author

ph commented Sep 18, 2015

@jsvd looking at it, I guess we have it for a while. ;)

@ph
Copy link
Contributor Author

ph commented Sep 18, 2015

I can reproduce it with your seed. <3

@ph
Copy link
Contributor Author

ph commented Sep 18, 2015

@jsvd it was in a middle of a crash, I have changed the code to use stoppable_sleep and added a rescue IOError when trying to close the socket, because its totally possible to try to close a socket which is in an inconsistent state.

jsvd
jsvd reviewed Sep 21, 2015
View reviewed changes
lib/logstash/inputs/gelf.rb
end
finished
def stop
super
Copy link
Member

Choose a reason for hiding this comment

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

super is no longer needed

@ph
Copy link
Contributor Author

ph commented Sep 21, 2015

Weird, non deterministic errors are back, investigating.

@ph
Copy link
Contributor Author

ph commented Sep 21, 2015

ready for another round of review @jsvd

jsvd
jsvd reviewed Sep 21, 2015
View reviewed changes
spec/inputs/gelf_spec.rb

describe "inputs/gelf" do
Thread.abort_on_exception = true
Copy link
Member

Choose a reason for hiding this comment

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

is this really needed?

@jsvd
Copy link
Member

jsvd commented Sep 21, 2015

Minor comment, otherwise LGTM. squash and merge 🐑 it

@ph
Refactor the shutdown
134965d 
This PR introduces the changes needed to follow the new shutdown
semantic, it also remove the gelf as a runtime dependencies and move it
as a development dependency since its only used in the tests.

Fixes logstash-plugins#17
@elasticsearch-bot
Copy link

Merged sucessfully into master!

@ph ph closed this in 1b3efd7 Sep 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
shutdown semantics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ph @jsvd @elasticsearch-bot @purbon @suyograo