Skip to content

Conversation

tobek
Copy link

@tobek tobek commented Jun 26, 2014

Also documenting the "reconnecting" event.

Happy to discuss this further, but either way, when working with sharded Redis instances (for instance using https://github.com/blindsey/redis-shard) it's helpful to have some way to know when a Redis server seems to have totally gone down so that we can send out alerts, remove it from the cluster, spin up another server, etc.

@ghost
Copy link

ghost commented Jul 8, 2014

👍

@brycebaril
Copy link
Contributor

How about emitting an error instead of a "broken" event, then we will get the throw behavior. This won't do anything if nobody is listening for the "broken" event.

@tobek
Copy link
Author

tobek commented Jul 9, 2014

That sounds good to me too, I only went for a special broken event because the comments talk about a "redis is broken mode", which suggests this is not so much an error as a change in state. I'm not fussed though, just need to able to listen for them somehow. error instead, then?

@brycebaril
Copy link
Contributor

Yeah, an "error" with an error something to the effect of new Error("Redis connection in broken state") or similar would be great!

@tobek tobek changed the title Adding "broken" event for when connection permanently goes down Emitting error event when connection permanently goes down Jul 14, 2014
@tobek
Copy link
Author

tobek commented Jul 14, 2014

Sounds good - done, and squashed into single commit.

…timeout

Also documenting the "reconnecting" event
@PizzaBrandon
Copy link

This appears to be an easily-fixed hole in the library. I provide a connection timeout option to the client and I need my program to know when the Redis client has given up; today it cannot know this without regularly inspecting the client object's retry_totaltime property. Please merge this and update in npm.

@aryehischechter
Copy link

this fixes issue #586

@@ -508,8 +508,10 @@ RedisClient.prototype.connection_gone = function (why) {
if (this.max_attempts && this.attempts >= this.max_attempts) {
this.retry_timer = null;
// TODO - some people need a "Redis is Broken mode" for future commands that errors immediately, and others
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove both TODOs including the console.error

@BridgeAR
Copy link
Contributor

BridgeAR commented Sep 3, 2015

This needs a rebase and two small comments otherwise LGTM

@tobek
Copy link
Author

tobek commented Sep 15, 2015

#829 has got this covered, closing this PR

@tobek tobek closed this Sep 15, 2015
BridgeAR added a commit that referenced this pull request Sep 16, 2015
Implement redis connection broken mode and more shiny things

Fixes #569
Fixes #587
Fixes #566 
Fixes #586 
Fixes #280 

This includes the fixes as suggested in #671, #615 and #533. Thx a lot to @qdb, @tobek and @chrishamant 

Closes #675, #463, #362, #438 and #724
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants