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

Add auto retry mechanism on connection failure #133

Merged
merged 8 commits into from Aug 17, 2014
27 changes: 25 additions & 2 deletions lib/cequel/metal/keyspace.rb
Expand Up @@ -21,6 +21,8 @@ class Keyspace
attr_reader :hosts
# @return Integer port to connect to Cassandra nodes on
attr_reader :port
# @return Integer maximum number of retries to reconnect to Cassandra
attr_reader :max_retries
# @return [Symbol] the default consistency for queries in this keyspace
# @since 1.1.0
attr_writer :default_consistency
Expand Down Expand Up @@ -98,6 +100,8 @@ def initialize(configuration={})
# single Cassandra instance to connect to
# @option configuration [Integer] :port (9042) port on which to connect
# to all specified hosts
# @option configuration [Integer] :max-retries maximum number of retries
Copy link
Member

Choose a reason for hiding this comment

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

I think you mean :max_retries not :max-retries

Copy link
Member

Choose a reason for hiding this comment

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

Oh I guess you do mean :max-retries but :max_retries seems more idiomatic no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it matters—I can do either it either way. If you feel strongly about it, I have no problem changing it to an underscore.

# on connection failure
# @option configuration [Array<String>] :hosts list of Cassandra
# instances to connect to (hostnames only)
# @option configuration [String] :username user to auth with (leave blank
Expand All @@ -115,9 +119,11 @@ def configure(configuration = {})
@configuration = configuration

@hosts, @port = extract_hosts_and_port(configuration)
@credentials = extract_credentials(configuration)
@credentials = extract_credentials(configuration)
@max_retries = extract_max_retries(configuration)

@name = configuration[:keyspace]

# reset the connections
clear_active_connections!
end
Expand Down Expand Up @@ -153,14 +159,27 @@ def client
#
# Execute a CQL query in this keyspace
#
# If a connection error occurs, will retry a maximum number of
# time (default 3) before re-raising the original connection
# error.
#
# @param statement [String] CQL string
# @param bind_vars [Object] values for bind variables
# @return [Enumerable] the results of the query
#
# @see #execute_with_consistency
#
def execute(statement, *bind_vars)
execute_with_consistency(statement, bind_vars, default_consistency)
retries = max_retries

begin
execute_with_consistency(statement, bind_vars, default_consistency)
rescue Cql::NotConnectedError, Ione::Io::ConnectionError => e
@raw_client = nil
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this will have the intended effect since @client will still be set in most cases. I think this line should call clear_active_connections! which should clear both the @raw_client and the @client.

Copy link
Member

Choose a reason for hiding this comment

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

As a meta-point, I'm not thrilled about having both the @raw_client and @client instance variables, which hold the same object but with different guarantees about its state. Maybe not worth worrying about in this PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't look closely enough for the clear_active_connections! method. I will make that change.

raise e if retries < 0
Copy link
Member

Choose a reason for hiding this comment

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

raise e can be shortened to just raise, and that frees you from needing to capture the exception variable on line 177 as well

retries -= 1
retry
end
end

#
Expand Down Expand Up @@ -269,6 +288,10 @@ def extract_hosts_and_port(configuration)
def extract_credentials(configuration)
configuration.slice(:username, :password).presence
end

def extract_max_retries(configuration)
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem strictly necessary to have this extracted into a method but I don't feel strongly one way or another.

configuration.fetch(:"max-retries", 3)
end
end
end
end
31 changes: 30 additions & 1 deletion spec/examples/metal/keyspace_spec.rb
Expand Up @@ -82,7 +82,36 @@
port: Cequel::SpecSupport::Helpers.port,
keyspace: "totallymadeup"

expect(nonexistent_keyspace.exists?).to be_false
expect(nonexistent_keyspace.exists?).to be false
end
end

describe "#execute" do
let(:statement) { "SELECT id FROM posts" }

context "without a connection error" do
it "executes a CQL query" do
expect { cequel.execute(statement) }.not_to raise_error
end
end

context "with a connection error" do
after(:each) do
RSpec::Mocks.proxy_for(cequel).reset
end

it "reconnects to cassandra with a new client after first failed connection" do
allow(cequel).to receive(:execute_with_consistency)
.with(statement, [], nil)
.and_raise(Ione::Io::ConnectionError)
.once

expect(cequel).to receive(:execute_with_consistency)
.with(statement, [], :quorum)
.once

expect { cequel.execute(statement) }.not_to raise_error
end
end
end
end
3 changes: 3 additions & 0 deletions templates/config/cequel.yml
Expand Up @@ -3,11 +3,13 @@ development:
host: '127.0.0.1'
port: 9042
keyspace: <%= app_name %>_development
max-retries: 3

test:
host: '127.0.0.1'
port: 9042
keyspace: <%= app_name %>_test
max-retries: 3

production:
hosts:
Expand All @@ -18,3 +20,4 @@ production:
keyspace: <%= app_name %>_production
username: 'myappuser'
password: 'password1'
max-retries: 3