-
Notifications
You must be signed in to change notification settings - Fork 39
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
[Client-1432] creates minimum connections on client startup #117
Conversation
lib/aerospike/cluster.rb
Outdated
@@ -63,6 +58,13 @@ def initialize(policy, hosts) | |||
end | |||
|
|||
initialize_tls_host_names(hosts) if tls_enabled? | |||
|
|||
@min_connections_per_node = policy.min_connections_per_node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This information is already in the client_policy
. No need to define new class variables to hold it.
lib/aerospike/cluster.rb
Outdated
@max_connections_per_node = policy.max_connections_per_node | ||
|
||
if @min_connections_per_node > @max_connections_per_node | ||
raise Aerospike::Exceptions::Aerospike.new(Aerospike::ResultCode::PARAMETER_ERROR, "Invalid connection range") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more descriptive error message would be more helpful.
lib/aerospike/cluster.rb
Outdated
@@ -584,7 +586,9 @@ def remove_alias(aliass) | |||
end | |||
|
|||
def create_node(nv) | |||
::Aerospike::Node.new(self, nv) | |||
node = ::Aerospike::Node.new(self, nv) | |||
node.create_min_connections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can pass the number to the method. That would remove the need to open the cluster class variables to the node. It would also make that method more flexible.
lib/aerospike/node.rb
Outdated
@@ -55,6 +55,7 @@ def initialize(cluster, nv) | |||
@replica_index = Atomic.new(0) | |||
@racks = Atomic.new(nil) | |||
|
|||
@min_connections = cluster.min_connections_per_node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary. See prior comment.
lib/aerospike/node.rb
Outdated
@@ -69,6 +70,17 @@ def has_rack(ns, rack_id) | |||
racks[ns] == rack_id | |||
end | |||
|
|||
def create_min_connections | |||
current_number_of_connections = @connections.length | |||
if @min_connections > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directly use cluster.client_policy.min_connections
.
@@ -18,7 +18,7 @@ module Aerospike | |||
|
|||
class Pool #:nodoc: | |||
|
|||
attr_accessor :create_proc, :cleanup_proc, :check_proc | |||
attr_accessor :create_proc, :cleanup_proc, :check_proc, :max_size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this value initialized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I didn't notice that It does not have tests on the first review, but it is ok otherwise.
No description provided.