Failover option, closes #821, closes #830, closes #831

[epoberezkin]

@BridgeAR This implements all the discussed features within failover option:

1. Switching to another host when connection fails
2. Switching to another host when it becomes readonly (with additional failover.readonly option) (Multiple connection strings for master-slave demotion/promotion. #830)
3. Seamlessly authenticating when the password was enabled in redis without failing the command (Handle enabling/changing redis password without application restart #821)
   4 Supporting multiple passwords per host so that when the password is changed and the driver reconnects the connection is successful (Handle enabling/changing redis password without application restart #821)

It includes all the functionality from #831

After the last merge from the upstream I have difficulties running tests locally, I expect the tests to fail for this PR - will be fixing them.

Help is appreciated :)

[BridgeAR]

@epoberezkin I'm really busy at the moment and could not do a review yet. I'll try to do a review this weekend. Thx for your work so far :)

[epoberezkin]

Thank you for the reply, no problem. I will be fixing the failing tests over the weekend. The review would be great. Maybe you can give me the advice what is the best environment to run the tests? Because I do it on mac and although I disable lots of things (ipv6, hiredis) I still have plenty of tests failing (even without my changes)...

[BridgeAR]

@epoberezkin I'm uncertain why the tests fail for you on mac. What node.js version do you use? They definitly work on linux properly and @paddybyers ran the tests on mac too.

[paddybyers]

@epoberezkin which tests are failing?

I had quite a few failures, especially relating to unix socket connection, because I had a redis process already running. When that was killed and the tests started redis for themselves, with the correct permissions on /tmp/redis.sock, then it helped.

[epoberezkin]

@paddybyers @BridgeAR redis definitely wasn't running but that's embarrassing really - now all the tests (without my changes yet) pass. It seems that updating redis wasn't enough - it also needed restart. I suspect that the changes I was making to make it work with older version (like removing ipv6, hiredis etc.) were preventing the newer one from working correctly... Thank you, now I can fix my changes.

[epoberezkin]

@BridgeAR seems like it is all working now, at least in travis... When you have some time to have a look please let me know what needs to be changed. @paddybyers thanks for the hint - the wrong socket path used for the second instance of redis was the main issue.

[epoberezkin]

Not sure why windows tests are failing... It seems like they were failing for 2.2.5 too.

[BridgeAR]

@epoberezkin the tests worked in windows too. There might still be a flaky test but it's only about one out of 10 builds that fail (like here).

[epoberezkin]

@BridgeAR the problem seems to be that it's not easy to determine in windows whether the redis has started. So the main instance is just assumed to have been started and both auth and rename tests are just skipped in windows. I guess I should just skip failover tests in windows too...

[BridgeAR]

@epoberezkin sounds ok for now.

@bcoe do you think you might find the time to get more tests to work on windows? :)

[epoberezkin]

@BridgeAR now it passes tests in most windows jobs: https://ci.appveyor.com/project/bcoe/node-redis/build/292

[epoberezkin]

OS: Default Azure; Environment: nodejs_version=0.12 - only one test failed
OS: Default Azure; Environment: nodejs_version=3.0 - tests failed to start (it says some package is missing)

[epoberezkin]

@BridgeAR I have merged master branch. All the windows tests pass apart from one test in connection.spec.js in the job "OS: Windows Server 2012 R2; Environment: nodejs_version=0.10"

Could you please merge it?

[BridgeAR]

I'll look through everything later on. Right now I'm abroad and am only rarely on the computer.

[epoberezkin]

Thank you, no problem

[epoberezkin]

@BridgeAR hi. How are we doing with this PR? I hope you can look at it some time this week :)

[lsde]

+1 for merge

[BridgeAR]

I had a glimpse but this is a significant change and I do not find the time to fully review this atm :(

If anyone has the time to do a review, I'd really appreciate the help. @NodeRedis/contributors @lsde

[epoberezkin]

@BridgeAR Thanks for pushing it. It's not a very small change indeed. On the other hand most of the changes are isolated and most of the added lines are additional tests - I've kept the changes to the core of the driver as minimal as possible.

[badboy]

First off: I'm not using node_redis that much myself, but I used it once in a while on smaller projects. I also talked with @BridgeAR about some improvements and about this PR as well.

I'm not convinced your changes belong into the core of node_redis.
It allows for even more configuration (I'm not a fan of providing knobs for every little detail in a client, @BridgeAR slightly disagrees with me on this one though).
It adds quite a bit of code, though most of it separated. That still means that code has to be maintained.
It is yet another mechanism for failover, apart from what the Redis project itself recommends for this use case: Sentinel.
It might very well be implemented on top of the basic node_redis (at least I expect it to be possible)

[epoberezkin]

@badboy Thanks for the reply.

I've done this PR as the result of the discussion in another PR: #831. Could you please read it?

The problem solved there (changing/enabling redis passwords #821) is not addressed in Sentinel or anywhere else and it can't be fully covered by error-handling function, as @BridgeAR wrote there.

After I've done #831, that was much simpler and smaller change, @BridgeAR specifically said that his preference is a more generic failover option, that is in this PR.

I don't mind either approaches as long as the original problem can be addressed in some way.

This PR also resolves issue #830. Having a failover mechanism in the library can be preferable to many users - it may be easier to manage. But I don't mind going back to the original PR, it solves my problems too...

I look forward to some progress here :)

[tombburnell]

+1
Hi, we are very keen for this change. We need to migrate to password enabled Redis and cannot afford the downtime. Please can this be reviewed and included as soon as feasible.

[BridgeAR]

@tombburnell just run the auth command with the upcoming password. The password is going to be saved in the client and when you switch restart the redis servers with the new config and password the auth command is going to be fired with the saved password and is therefor able to connect without further downtime.