Redis Error on Version < 2.6.12

[hellerve]

I don't know whether this should be reported here as I have yet dive into the mechanics of this package, but I wanted to report that due to the changed interface of the SET command in Redis since 2.6.12 (see here), OpenConnection will panic immediately with the error:

rmq redis error is not nil ERR wrong number of arguments for 'set' command

If used with any redis version below 2.6.12. A note about that in the README (or a fix if possible, prettypleasemaybe?) would be really helpful.

Cheers

PS: A first glance at the package tells me that this panic is due to the mechanics of hearbeating. I haven't fully understood the package yet, though, so take my analysis with a saltshaker worth of salt.

[hellerve]

Update: Looking into it some more, I see that in fact it is due to heartbeating; the optional seconds argument is invalid in version below 2.6.11. If possible & desired, this can be changed to SetEX without any breakage. Now, the Go client for Redis, does not provide a SetEX command, but it DOES provide SetNX, which will amount to a SetEx call if the specified argument is not of nanosecond precision.

I think I could fix that if the maintainers & users of this library think this is a good idea.

[hellerve]

I fixed this (apparently) in the current commit in my fork. It is a kind of yucky four-line fix, because SetNX does not actually dispatch a SetNX command. Meh.

[wellle]

@hellerve: Hi, I might look into this again this weekend.

But I can already say that SetNX is probably not what we want for the heartbeat. It's important that we update the TTL every time. If we only set it with a TTL if it does exist, then we have gaps between expiration and setting it again which will be observable by the cleaner. So if the cleaner happens to check a heartbeat in that window it will clean the consumer even though it should be valid. The job of the heartbeat is to always keep this key set as long as the consumer is alive.

Quick general question: Did you consider updating your Redis to a more recent version that supports the new Set call?

[hellerve]

I would like to, but as I am in a relatively constrained environment, this might not happen any time soon (especially as this is the first time this is a problem).

Thank you for the quick response. I did not really look into it all that deeply, but what you are saying makes sense. I will still keep my fork until this is resolved, just for reference.

[wellle]

I will still keep my fork until this is resolved, just for reference.

I hope you're not handling any important deliveries then. I would strongly discourage you from doing that.

Having the cleaner clean up an active consumer might lead to inconsistencies like double delivery. Not using a cleaner at all will not redeliver unacked packages when the consumer gets restarted.

What I would suggest for a quick fix is to go back to upstream on your fork and try to revert this commit https://github.com/adjust/rmq/commit/78b24c449928d1e8a0fda059700ad6705d709c90 and possibly make the needed adjustments. That way you use a redis client that should work with your old Redis version.

[hellerve]

Thanks for the suggestion. Right now I am in the process of figuring things out, so that last comment saves me from potential headaches regarding my delivieries.

If you could look into how to best resolve this I would much appreciate that, although I understand it's not high on the priority list to support legacy versions of Redis.

[svperfecta]

Hey all - Just wondering, is this still a problem? I think redis is at 4.0 now.

[hellerve]

I cannot confirm or dispel anything, as I don’t maintain any project using this particular setup anymore. Personally, I wouldn’t mind if you closed this issue, although I suppose it persists unless there were any significant changes since the last update in this thread.

[wellle]

@genexp: I believe the title of this issue was wrong, I just changed it to be in line with what the description is saying:

[...] will panic immediately [...] if used with any redis version below 2.6.12

So this is only a problem if you're using an ancient version of Redis. All versions from 2.6.12 on should be fine, including 4.0 of course.

[hellerve]

Oh yes, my bad! Sorry for the bad PR!

[svperfecta]

Ahh yep! #notworthfixing :P

[wellle]

Closing as there's nothing to be done here.