search

mbr / simplekv

A simple key-value store for binary data.
http://simplekv.readthedocs.io
MIT License
152 stars 50 forks source link

Swapping PSETEX for SETEX #25

Closed slick666 closed 9 years ago

slick666 commented 9 years ago

Swapping PSETEX for SETEX because we don't lose any functionality and make the code REDIS 2.4 compliant.

nelsnelson commented 9 years ago

Looks good to me. :+1:

mbr commented 9 years ago

The tests are failing (look at travis output) due to SETEX instead of PSETEX.

From the redis manual:

PSETEX works exactly like SETEX with the sole difference that the expire time is specified in milliseconds instead of seconds.

So what we lose is sub-second accuracy for expire times, whether or not that is something worth keeping around is a different matter, but breaking the tests is not okay =).

On Fri, Apr 3, 2015 at 6:21 PM, Nels Nelson notifications@github.com wrote:

Looks good to me. [image: :+1:]

— Reply to this email directly or view it on GitHub https://github.com/mbr/simplekv/pull/25#issuecomment-89342167.

slick666 commented 9 years ago

@mbr What you say makes sense. It's a limitation of Redis, depending on your version but I'm of two minds on how to solve it. Should this be something thats rolled out as a config value, or would it be better to fail-over to the SETEX.

mbr commented 9 years ago

Check 9ba4ddd. I can't really test Redis 2.4 support (or any specific redis version for that matter) because I have not found a way to convince travis to give me a specific version yet.

I've moved the burden back onto the caller. As long as you do not use non-integer values as ttyls, setex is used and everything is fine.

slick666 commented 9 years ago

I love it. I'll test it out Monday when I get back to work and cant verify in my dev env there. TravisCI does let you specify the distro so perhaps adding in centos6 would limit you to redis 2.4 which is the current latest and why I'm using 2.4