Add auto retry mechanism on connection failure #133
Conversation
NOTE: This will retry 10x before giving up. Credit to @pezra also for pairing.
…um retries that will be made to reconnect to cassandra * Added spec stubs * Modified template for config file (added max-retries)
|
@outoftime, any chance we could get some feed back on this? It looks good to me but then i helped write it. 😄 |
| @@ -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) | |||
| configuration[:"max-retries"] || 10 | |||
There was a problem hiding this comment.
Seems like 10 is a pretty big default, no? I would actually say let's have no retries by default, or if it makes sense to have a non-zero default, something in the ballpark of 2 or 3 seems better…
Also configuration.fetch(:"max-retries", 10) would be a bit better I think
There was a problem hiding this comment.
Sounds good. I'll make that change.
|
Implementation looks good in general, thanks for this! I left a couple of comments on specifics… |
|
@outoftime I updated the |
|
@outoftime, thoughts? |
| @@ -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 | |||
There was a problem hiding this comment.
I think you mean :max_retries not :max-retries
There was a problem hiding this comment.
Oh I guess you do mean :max-retries but :max_retries seems more idiomatic no?
There was a problem hiding this comment.
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.
…ctions!` instead of directly accessing ivar
|
@outoftime Thanks for you feedback. @pezra and I are eager to see this in the main branch and hopefully in a release soon. Do you feel like our work is sufficient for the code base? |
| @@ -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) | |||
| configuration.fetch(:"max_retries", 3) | |||
There was a problem hiding this comment.
No longer need the quotation marks around the symbol
|
Couple of tinnnny style comments inline. Knock those out and I'd be happy to merge. I am traveling without a laptop until the middle next week so won't be able to do a gem release until then. |
…ction_failure Add auto retry mechanism on connection failure
In development, I am often restarting my cassandra clusters and find that the cequel gem would not handle reconnecting gracefully without a manual reconnection.
This pull request provides a retry mechanism to auto-reconnect to cassandra a number of times specified by a
max-retriesconfig parameter that defaults to 10. It does this by clearing out the disconnectedraw_clientinstance inCequel::Metal::Keyspacewhen it detects a connection error.Thanks ahead of time for looking at this and feedback is appreciated.