Conversation
khaf
left a comment
khaf
left a comment
There was a problem hiding this comment.
In general, logging rescues should raise the exception afterwards.
The logic for max_connections_exceeded is faulty and will lead to an invalid state for the client in which it won't be able to create new connections.
| @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
| 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.
| 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)
| # 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.
| 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.
| @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}") |
| 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.
2 participants
This PR contains the following code change: