support enabling password in redis …

[epoberezkin]

... after connection is already established without application restart, issue #821

Please let me know if you think this is the right approach.
I will then implement the second part - support changing redis password.

Thank you.

[epoberezkin]

I will look into it... It is passing locally though.

[epoberezkin]

The test was failing because I had a different error message in my local redis version when authentication failed. It is all fixed and working now.

[epoberezkin]

Both parts of issue #821 are now implemented.
It would be great if you could merge it...
Thanks!

[epoberezkin]

@BridgeAR Hi Ruben. It would be great if you could merge it and push to npm... All the tests pass, not sure why coveralls shows as pending. We really need it in another package (connect-redis) and using forks is a bad option because we have many apps that use it... Please let me know if something needs to be changed/fixed. Thanks.

[BridgeAR]

@epoberezkin I only had a glimpse at the code earlier. I played around with it and it seems to me that this is a very special case and it should likely be handled in your code. I changed a tiny bit in your test code so it works just as you want it to behave. Have another look at it.
So a solution as I see it might be that you use a small helper function to do what you wish?

But maybe the others are of other opinion. And I'm way to tired to think properly anymore. It's very early in the morning here.

[epoberezkin]

@BridgeAR thanks for the response. I fully understand the need to keep what's not common outside to avoid bloating it :)

TL;DR Changing redis password without disrupting the application is a very common need for us and many others. Doing it outside of driver is a real pain, whichever way you do it.

That may be not a situation that happens often, but it's a very common need to change redis password (depending on the security policy you may need to do it on a regular basis) and it is really difficult to manage within software as software can't really control the change of redis password, it can only adapt to it.

The test simply mimics what happens outside of the application. In reality the change of password is done manually, from redis-cli. The reason for that is that multiple apps are using the same redis, so none of them can initialte the change of the password without breaking the other. For example, one app uses connect-redis to store sessions and another app accesses those sessions using this redis client.

The difficulty of handling it in the code is that every command in its callback would need to handle the password change. So the only other solution apart from implementing the feature in the driver is to use the wrapper that would implement all commands and messages that redis driver does. It's much more complex than the change in the driver and also has performance cost.

Given that we have more than 10 redis instances and roughly twice more node apps that use them it is a real pain to manage it without this change.

[jasoniangreen]

👍

[BridgeAR]

After giving this another thought I do see the need for it but I'd prefer a more generic way of handling something like this.

I thought about adding a "callFunctionOnError" option that you'd pass a specific error too as parameter and that function would be called if that error occurs. That way it should be possible to implement this but also a lot of other things. What do you think @epoberezkin ? :)

[epoberezkin]

@BridgeAR I think it would do the trick as long as the failed command will be retried after the error is handled. But maybe it's a bit too generic solution. I'd prefer if it was managed via configuration. How do feel about covering both this issue and #830 with supplying array of connection settings (including host, port and password) that would all be tried in case of certain errors (like read-only, not authenticated and maybe some others)?

[epoberezkin]

@BridgeAR it would be really good to decide what we do about this features... Not that we are in a great rush but we really need it for several projects. Maybe we could merge this? (I would resolve conflicts then). Or I can implement multiple connection settings from #830 that is a bit more generic. Probably there should still be baseline settings and the array of settings that would extend the baseline.

[BridgeAR]

@epoberezkin sry, somehow your reply slipped through. This week is a bit busy for me and if this could still wait a few more days that would be great.

[epoberezkin]

Thanks for the reply. I can implement myself, I only need some decision about what to do :) But a few days is not a problem of course.

[epoberezkin]

@BridgeAR hi, sorry for chasing, any update on this?

[BridgeAR]

I'll invest some time into it later today.

[epoberezkin]

thanks!

[BridgeAR]

@epoberezkin I'm working on a suggestion and I'll have it done tomorrow. Either we go for that, if you like that suggestion or you could implement the multiple connection settings otherwise. That way we could see what fits bests? :)

[epoberezkin]

cool, looking forward for it

[BridgeAR]

@epoberezkin it took me a while to get to it, sry. I played around with implementing a generic function and it works quite nice in the way I thought about it (you can provide any commands in that function and if you want to you can repeat the original call when ever you want) but I could not fix all your wishes with it. And since it's really very generic you can also do a lot of stuff that might have side effects that someone did not think about. So you might be right that a more specific solution is the better one here.

Would you give a pull request for multiple connections a shot? :)

[epoberezkin]

@BridgeAR No problem. I will, definitely do. Just to confirm, the syntax for options object I suggest is this:

failover: {
  connections: [
    {
      host: '...',
      port: '...',
      auth_pass: '...'
      // , ...
    },
    {
      host: '...',
      port: '...',
      auth_pass: '...'
      // , ...
    }
    // , ...
  ]
}

This way some additional options can be added inside failover in the future. If any of connection parameters are not passed in some of connections, the main connection parameters are used. Also, regardless whether the main connection settings are included inside failover or not, driver should be able to switch back to them. Also if no connection parameters are used on the top level in options it should use parameters from inside failover option... Something like this.

What do you think?

[BridgeAR]

I like it. It's likely the best way to have mutiple alternatives. I'm uncertain about not passing any connection parameters but that's more of a detail in my opinion.

[epoberezkin]

Ok, thanks, I'll work on it some time this week

[BridgeAR]

@epoberezkin I guess you were busy so far? :)

[epoberezkin]

@BridgeAR It's slower than I'd like to, but I hope to finish it this week :) I've just finished options and general failover utils. If you're interested you can see the progress in the branch https://github.com/MailOnline/node_redis/commits/failover (I will squash all commits to master once it's working and make a PR with one commit). But it's far from finished still :)

I actually have a question regarding client.retry_totaltime. It seems like every time client tries to reconnect this property is increased. But once reconnection succeeds it is not reset to 0. I think it should be reset, as otherwise after some number of connection losses the driver will potentially not try to reconnect for a long enough time because this property will become equal to the allowed maximum. Of course with the default settings it may not happen soon... But with failover support the reconnection becomes a more normal thing so it will be more likely.

I can fix it within the same PR if you agree (reset retry_totaltime to 0 after successful reconnect). Please let me know what you think, I may be missing something.

[epoberezkin]

Sorry, disregard the question, I can see it is reset :)

[epoberezkin]

closing it now, there is the new PR #895