[Client-618] max connections enforced #114
Conversation
| @@ -487,6 +487,9 @@ def execute | |||
| @node = get_node | |||
| @conn = @node.get_connection(@policy.timeout) | |||
| rescue => e | |||
| if e.is_a?(Aerospike::Exceptions::MaxConnectionsExceeded) | |||
| Aerospike.logger.error("Maximum connections established. No new connection can be created. #{e}") | |||
There was a problem hiding this comment.
Same issue. Logging without re-raising the exception.
There was a problem hiding this comment.
Have not re-raised exception here as there is a retry loop here
| @@ -17,19 +17,27 @@ | |||
| module Aerospike | |||
| class ConnectionPool < Pool | |||
|
|
|||
| attr_accessor :cluster, :host | |||
| attr_accessor :cluster, :host, :number_of_creations | |||
There was a problem hiding this comment.
I Believe we can use a better name. :total_conns, etc.
| else | ||
| conn = cluster.create_connection(host) | ||
| if conn.connected? | ||
| @number_of_creations += 1 |
There was a problem hiding this comment.
Where is this number decreased? You will not create new connections after max_size connections are disconnected and new connections are needed.
| @@ -49,6 +51,17 @@ | |||
| end | |||
| end | |||
|
|
|||
| context "enforce max connections as a hard limit" do | |||
There was a problem hiding this comment.
This test is good for the expected behavior, but does not consider what happens when a few connections are closed. Will the client allow new connections to be created afterwards, or will be stuck at max_conns_exceeded forever? (Which from the code, I think it does)
| @@ -1,4 +1,4 @@ | |||
| # encoding: utf-8 | |||
| module Aerospike | |||
| VERSION = "2.26.0" | |||
| VERSION = "2.26.1" | |||
There was a problem hiding this comment.
Should be 2.27.0. Too many deep changes have been applied.
| conn.write(@data_buffer, @data_offset) | ||
| conn.read(@data_buffer, HEADER_SIZE) | ||
| node.put_connection(conn) | ||
| rescue => e | ||
| conn.close if conn | ||
| node.close_connection(conn) if conn |
There was a problem hiding this comment.
The better design should be adding a node attribute to the connection, adding the clean up code there if the node is set (node should be set after a successful connection is established to the database node), and then adding a self.finalize to the Connection class and setting it up in the initializer. That way, even a user takes a connection from the pool and forgets to put it back, the code will still work.
| @@ -487,6 +487,9 @@ def execute | |||
| @node = get_node | |||
| @conn = @node.get_connection(@policy.timeout) | |||
| rescue => e | |||
| if e.is_a?(Aerospike::Exceptions::MaxConnectionsExceeded) | |||
| Aerospike.logger.error("Maximum connections established. No new connection can be created. #{e}") | |||
| @@ -224,14 +224,18 @@ def refresh_reset | |||
| Node::Refresh::Reset.(self) | |||
| end | |||
|
|
|||
| def close_connection(conn) | |||
There was a problem hiding this comment.
Not needed if the suggested changes are applied.
| end | ||
|
|
||
| # Destroys the connection pool and closes all the connections. | ||
| def self.finalize(id) |
There was a problem hiding this comment.
Method should be set during initialization to be called on GC:
ObjectSpace.define_finalizer(self, self.class.method(:finalize))
| Aerospike.logger.error("Error occurred while closing a connection") | ||
| raise e | ||
| end | ||
| @total_connections -= 1 |
There was a problem hiding this comment.
This code is buggy. I'll leave it as an exercise for you to find out.
This PR contains the following code change: