Skip to content

Commit

Permalink
[CLIENT] allow transports to close connections during __rebuild_conne…
Browse files Browse the repository at this point in the history
…ctions

adds a no-op __close_connections called during __rebuild_connections
this commit also implements this method for manticore transport that marks
all connections as dead and calls Manticore::Client#close for each connection

Closes #241
See: cheald/manticore#39
Related: #245, logstash-plugins/logstash-output-elasticsearch#306
  • Loading branch information
jsvd committed Dec 2, 2015
1 parent 97ab1a8 commit 7ba1fbd
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 0 deletions.
Expand Up @@ -96,9 +96,18 @@ def resurrect_dead_connections!
def __rebuild_connections(arguments={})
@hosts = arguments[:hosts] || []
@options = arguments[:options] || {}
__close_connections
@connections = __build_connections
end

# Closes the connections collection.
#
# @api private
#
def __close_connections
# to be implemented by specific transports
end

# Log request and response information.
#
# @api private
Expand Down
Expand Up @@ -105,6 +105,16 @@ def __build_connections
:selector => options[:selector]
end

# Closes all connections by marking them as dead
# and closing the underlying HttpClient instances
#
# @return [Connections::Collection]
#
def __close_connections
@connections.each {|c| c.dead! }
@connections.all.each {|c| c.connection.close }
end

# Returns an array of implementation specific connection errors.
#
# @return [Array]
Expand Down
5 changes: 5 additions & 0 deletions elasticsearch-transport/test/unit/transport_base_test.rb
Expand Up @@ -473,6 +473,11 @@ def initialize(*); end
@transport = DummyTransport.new
end

should "close connections" do
@transport.expects(:__close_connections)
@transport.__rebuild_connections :hosts => ['foo']
end

should "should replace the connections" do
assert_equal [], @transport.connections
@transport.__rebuild_connections :hosts => ['foo', 'bar']
Expand Down
13 changes: 13 additions & 0 deletions elasticsearch-transport/test/unit/transport_manticore_test.rb
Expand Up @@ -26,6 +26,19 @@ class Elasticsearch::Transport::Transport::HTTP::ManticoreTest < Test::Unit::Tes
assert_instance_of ::Manticore::Client, @transport.connections.first.connection
end

should "implement __close_connections" do
assert_equal 1, @transport.connections.size
@transport.__close_connections
assert_equal 0, @transport.connections.size
end

should "prevent requests after __close_connections" do
@transport.__close_connections
assert_raises ::Manticore::ClientStoppedException do
@transport.perform_request 'GET', '/'
end
end

should "perform the request" do
@transport.connections.first.connection.expects(:get).returns(stub_everything)
@transport.perform_request 'GET', '/'
Expand Down

9 comments on commit 7ba1fbd

@karmi
Copy link
Contributor

@karmi karmi commented on 7ba1fbd Dec 2, 2015

Choose a reason for hiding this comment

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

👍

@cnocito
Copy link

Choose a reason for hiding this comment

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

I think this change introduce an error:

{:timestamp=>"2015-12-18T15:11:28.905000+0000", :message=>"undefined method close' for #<Manticore::Client:0x16506b32>", :class=>"NoMethodError", :backtrace=>["/opt/logstash/vendor/bundle/jruby/1.9/gems/elasticsearch-transport-1.0.15/lib/elasticsearch/transport/transport/http/manticore.rb:115:in__close_connections'", "org/jruby/RubyArray.java:1613:in each'", "/opt/logstash/vendor/bundle/jruby/1.9/gems/elasticsearch-transport-1.0.15/lib/elasticsearch/transport/transport/http/manticore.rb:115:in__close_connections'", "/opt/logstash/vendor/bundle/jruby/1.9/gems/elasticsearch-transport-1.0.15/lib/elasticsearch/transport/transport/base.rb:99:in __rebuild_connections'", "/opt/logstash/vendor/bundle/jruby/1.9/gems/elasticsearch-transport-1.0.15/lib/elasticsearch/transport/transport/base.rb:77:inreload_connections!'", "/opt/logstash/vendor/bundle/jruby/1.9/gems/logstash-output-elasticsearch-2.2.0-java/lib/logstash/outputs/elasticsearch/http_client.rb:85:in sniff!'", "/opt/logstash/vendor/bundle/jruby/1.9/gems/logstash-output-elasticsearch-2.2.0-java/lib/logstash/outputs/elasticsearch/http_client.rb:73:instart_sniffing!'", "org/jruby/ext/thread/Mutex.java:149:in synchronize'", "/opt/logstash/vendor/bundle/jruby/1.9/gems/logstash-output-elasticsearch-2.2.0-java/lib/logstash/outputs/elasticsearch/http_client.rb:73:instart_sniffing!'", "org/jruby/RubyKernel.java:1479:in loop'", "/opt/logstash/vendor/bundle/jruby/1.9/gems/logstash-output-elasticsearch-2.2.0-java/lib/logstash/outputs/elasticsearch/http_client.rb:72:instart_sniffing!'"], :level=>:error}

@karmi
Copy link
Contributor

@karmi karmi commented on 7ba1fbd Dec 18, 2015

Choose a reason for hiding this comment

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

@cnocito That would be weird, see the tests. Note that it relies on a fairly nws version of the manticore library, which one do you have?

@jsvd
Copy link
Member Author

@jsvd jsvd commented on 7ba1fbd Dec 18, 2015

Choose a reason for hiding this comment

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

I think it's a problem with the Gemfile, which should have bumped the minimum required version for the manticore gem :(

@cnocito
Copy link

Choose a reason for hiding this comment

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

/opt/logstash/vendor/bundle/jruby/1.9/gems/manticore-0.4.4-java/ext/manticore
/opt/logstash/vendor/bundle/jruby/1.9/gems/manticore-0.4.4-java/ext/manticore/org/manticore
/opt/logstash/vendor/bundle/jruby/1.9/gems/manticore-0.4.4-java/spec/manticore
/opt/logstash/vendor/bundle/jruby/1.9/gems/manticore-0.4.4-java/lib/manticore

@jsvd
Copy link
Member Author

@jsvd jsvd commented on 7ba1fbd Dec 18, 2015

Choose a reason for hiding this comment

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

to correct my previous statement: this is not about the Gemfile or gemspec specifying the wrong version of manticore.
Since the transport is selectable (between manticore, faraday, curb), there is no runtime dependency on these transport gems. Which creates a problem in this scenario where we need to ensure at least manticore version X is used IF you select it as your transporter.
A workaround is to check the gem version when doing a require and abort, but that doesn't fix the dependency resolution at install time..

@cnocito
Copy link

Choose a reason for hiding this comment

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

Ok, I agree with jsvd. Latest Manticore version is 0.5.2, the logstash installer comes with 0.4.4... I yum installed today, something needs to be updated for this fix.

@karmi
Copy link
Contributor

@karmi karmi commented on 7ba1fbd Dec 18, 2015

Choose a reason for hiding this comment

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

@cnocito We'll have a look at tightening up the versions here, thanks for the report!!

@cnocito
Copy link

Choose a reason for hiding this comment

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

@karmi , you are welcome!

Please sign in to comment.