Using redis library v2.6 instead of 0.12, fixed bugs in connection logic

[wirehead]

This is both a bugfix and a redis library upgrade.

I was trying to set up a few independent instances of my app and I was wondering why the other redis-using code in my app was working fine when I set the database number in the path, but cache-service-redis was still trying to access database zero.

Redis 0.12 doesn't support database numbers, as it turns out. It's an old release at this point. So I decided to upgrade to the latest version of the Redis library, which lets you pass the Redis URL to the constructor directly, which has the nice effect of letting me remove the URL parsing in the cache service. This fixes the bug I'm experiencing while trying to install my app, which depends on cache-service-redis, in production.

I also fixed the deprecation warnings.

[jpodwys]

Thanks for the PR! I have a lot to get done within the next few days, but I'll make time for this before the end of the week.

[jpodwys]

Sorry for the wait and thanks for adding this.

In my original code, I was using the max_attempts config option to prevent more than 5 connection attempts but I don't see an equivalent in your changes. I see that you're checking options.times_connected and options.attempt in your retry_strategy function, but it appears to me that your usage of options.attempt won't limit connection attempts to 5.

Instead, your usage of options.attempt appears to increasingly delay connection attempts up to a maximum of 3 seconds between each with no limit to the number of attempts itself. Additionally, your usage of options.times_connected appears to prevent additional connection attempts after 5 successful connections have been established, not 5 failed attempts.

Can you confirm that my assumptions here are correct? If so, I'd prefer that you implement a check for a maximum number of options.attempt and/or a maximum options.total_retry_time so that connection attempts don't go on forever.

Thanks!

[jpodwys]

@wirehead any updates on this?

[wirehead]

I'll be digging into this in the next 2-3 days. Haven't gotten to it yet.

[jpodwys]

OK thanks for the fast response.

[wirehead]

OK, I spent a few minutes with the code. You were correct; I should have been checking options.attempt instead of options.times_connected to preserve the same behavior as the last version. In order to prevent future contributors from having to reason this out themselves, I added a unit test. :)

[jpodwys]

Thanks for the upgrade and the unit test additions!

[jpodwys]

Published to npm as 1.2.2.