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

renaming teardown to close in plugins #3952

Closed
35 of 40 tasks
jsvd opened this issue Sep 22, 2015 · 8 comments
Closed
35 of 40 tasks

renaming teardown to close in plugins #3952

jsvd opened this issue Sep 22, 2015 · 8 comments

Comments

@ph
Copy link
Contributor

ph commented Sep 22, 2015

I don't think this should be a problem, personally I haven't seen a lot of code with a method named teardown outside of unit testing.

@ph
Copy link
Contributor

ph commented Sep 22, 2015

We can always push a new version of a plugin if it doesn't work.

@purbon
Copy link
Contributor

purbon commented Sep 22, 2015

I agree with @ph it should be simple, but would not be less to do an small checkup.

@ph doing that you say in your last comment imply there are test that check for this in the plugin, this is now the case of more plugins, but still not all. we should be careful with the ones we bundle with.

what do you think?

@jsvd
Copy link
Member Author

jsvd commented Sep 22, 2015

what I can do is have a new logstash checkout of the 2.0 snapshot and devultils and run:

cd all_plugins
# get me all plugins with teardown in them
grep teardown . -r | cut -d ":" -f 1 | cut -d "/" -f 2 | uniq > teardown.txt
# rename all occurrences
find . -name '*.rb' -type f -exec sed -i '' "s/teardown/close/g" {} \;
# go through each plugin that was renamed
# commit changes
# modify gemfile to point to local logstash and devutils
# run specs
# if they pass push to master
for plugin in `cat teardown.txt`; do cd $plugin; git commit -a -m "renamed teardown to close"; cp ../Gemfile .; bundle install; bundle exec rspec && git push origin master; cd ..; done     

this way at least we know specs run, before commiting

@ph
Copy link
Contributor

ph commented Sep 22, 2015

I am OK with that @jsvd

@jsvd
Copy link
Member Author

jsvd commented Sep 22, 2015

output: https://gist.githubusercontent.com/jsvd/78830d687f676881ed35/raw/abb49b58b0a02c6c3336d2fa123af6ee6be902dc/gistfile1.txt

some plugins either failed or specs hanged, this was expected since some have shutdown related prs being reviewed

@jsvd
Copy link
Member Author

jsvd commented Sep 22, 2015

Plugins that still have teardown calls/method definitions in them:

./logstash-input-pipe/lib/logstash/inputs/pipe.rb:  def teardown
./logstash-input-tcp/lib/logstash/inputs/tcp.rb:  def teardown
./logstash-input-tcp/lib/logstash/inputs/tcp.rb:  end # def teardown
./logstash-input-tcp/spec/inputs/tcp_spec.rb:      plugin.teardown
./logstash-input-tcp/spec/inputs/tcp_spec.rb:      plugin.teardown
./logstash-output-elasticsearch_java/lib/logstash/outputs/elasticsearch_java.rb:    @retry_teardown_requested = Concurrent::AtomicBoolean.new(false)
./logstash-output-elasticsearch_java/lib/logstash/outputs/elasticsearch_java.rb:      while @retry_teardown_requested.false?
./logstash-output-elasticsearch_java/lib/logstash/outputs/elasticsearch_java.rb:  def flush(actions, teardown = false)
./logstash-output-elasticsearch_java/lib/logstash/outputs/elasticsearch_java.rb:  def teardown
./logstash-output-elasticsearch_java/lib/logstash/outputs/elasticsearch_java.rb:    @retry_teardown_requested.make_true
./logstash-output-elasticsearch_java/lib/logstash/outputs/elasticsearch_java.rb:  # and once that thread is ended during the teardown process, a final call 
./logstash-output-elasticsearch_java/lib/logstash/outputs/elasticsearch_java.rb:  # to this method is done upon teardown in the main thread.
./logstash-output-elasticsearch_java/spec/integration/outputs/retry_spec.rb:    subject.teardown
./logstash-output-elasticsearch_java/spec/integration/outputs/retry_spec.rb:    subject.teardown
./logstash-output-elasticsearch_java/spec/integration/outputs/retry_spec.rb:    subject.teardown
./logstash-output-elasticsearch_java/spec/integration/outputs/retry_spec.rb:    subject.teardown
./logstash-output-influxdb/lib/logstash/outputs/influxdb.rb:  def flush(events, teardown = false)
./logstash-output-influxdb/lib/logstash/outputs/influxdb.rb:  def teardown
./logstash-output-influxdb/lib/logstash/outputs/influxdb.rb:  end # def teardown
./logstash-output-jms/lib/logstash/outputs/jms.rb:def teardown
./logstash-output-neo4j/lib/logstash/outputs/neo4j.rb:  def teardown
./logstash-output-neo4j/spec/outputs/neo4j_spec.rb:    it "should teardown" do
./logstash-output-neo4j/spec/outputs/neo4j_spec.rb:      expect { output.teardown }.to_not raise_error
./logstash-output-s3/spec/outputs/s3_spec.rb:      s3.teardown
./logstash-output-s3/spec/outputs/s3_spec.rb:        s3.teardown
./logstash-output-s3/spec/outputs/s3_spec.rb:        s3.teardown

@ph
Copy link
Contributor

ph commented Sep 22, 2015

I will check the S3 output.

Vincent-- added a commit to RewardGateway/logstash-output-loggly that referenced this issue Apr 18, 2016
Vincent-- added a commit to RewardGateway/logstash-output-loggly that referenced this issue Jan 10, 2017
webmat pushed a commit to webmat/logstash-output-loggly that referenced this issue May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants