Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Will timeouts be considered to adding to the repo? #13

Closed
chowchow316 opened this issue Apr 5, 2017 · 14 comments
Closed

Will timeouts be considered to adding to the repo? #13

chowchow316 opened this issue Apr 5, 2017 · 14 comments

Comments

@chowchow316
Copy link
Contributor

I found that timeout are not implemented, is there any plan to implement it? And is there any technical challenge in the implementation? Do you accept the contribution from outside?
Thank you!

@alicebob
Copy link
Owner

alicebob commented Apr 6, 2017

The primary use case for miniredis is unittests, in which case timeouts are not wanted. I do want to test if timeouts are set correctly, but the timeouts shouldn't trigger. So I prefer to keep them as is.

What's your use case for the timeouts?

@chowchow316
Copy link
Contributor Author

Thanks for your reply. Currently I'm using Redis to implement a rate-limiting service, and trying to cover all the functionalities by unittests. And till now, only the expire part cannot be covered when using miniredis as a mock. Do you have any suggestions?

@alicebob
Copy link
Owner

I see you point, I'll think about it. It would certainly be optional behavior.

@chowchow316
Copy link
Contributor Author

chowchow316 commented Apr 11, 2017

Yes, and I'm thinking about adding this feature in my fork, my current idea is to add a periodic timer to trigger key deletion in the DB. How do you like it?

@alicebob
Copy link
Owner

alicebob commented Apr 12, 2017

The more I think about it the more I would make a sort of fast-forward, time-machine function. You call it with a duration, and all TTLs timeouts will be decreased by that much, and any expired keys will be deleted. In this way you then have full control in your tests over how 'fast' time goes. You can do things like:

<set redis keys with ttl of 7 days>
db.FastForward(24 * time.Hour)
<check that things didn't expire yet>
db.FastForward(6 * 24 *time.Hour)
<check that things expired>

Or you can call it in a quick loop in your test, if you want more real-time like behavior. I think having a system which is paused by default and only moves when you tell it to should work good for testing. And as a bonus you won't need to call time.Sleep() in your tests, which keeps them speedy and simple.

Some things to consider:

  • The TTL values are currently simple integers, and sometimes they are seconds, and sometimes they are milliseconds. You might want to add another map which keeps things as proper durations
  • Redis used to support absolute timestamps as TTLs, maybe that should simply not be supported here?
  • Maybe it is nicer to work with absolute times, and not with durations. Not sure.

How about this way, Sun?

@chowchow316
Copy link
Contributor Author

chowchow316 commented Apr 12, 2017

Hi Harmen, I like the idea of time machine, and for mock redis, the time manipulation be as flexible as possible. And If I understand your idea correctly, it should be somewhat like #14 (could you help to review it? Thanks! ).
Furthermore, there should also be a FastForward in miniredis.go, so that when people writing unittest they can call the function and manipulate time as they want.

Regarding your concerns, I think for unit test, durations should work fine. And I agree that TTL values can be extended with units (seconds, milliseconds, etc).

@alicebob
Copy link
Owner

Looks good as the general idea. Would it be useful this way in the mixer project?

@chowchow316
Copy link
Contributor Author

chowchow316 commented Apr 13, 2017

Thanks, and yes, it is useful for the unit test of our redis-based rate-limiter. And would you like to merge the PR in? If so, I will make it more complete.

@alicebob
Copy link
Owner

Yeah, I would like to merge it.

@chowchow316
Copy link
Contributor Author

Awesome, I will update the PR, and your comments would be a lot of help.

@alicebob
Copy link
Owner

I pulled your branch into the ttl branch from #15, and made some more small improvements to the expire handling.

Does this now work for you? If so I'll merge this into master and call it 2.0.

@alicebob
Copy link
Owner

It's merged, let me know if there are any issues!

@alicebob
Copy link
Owner

And thanks for the help!

@chowchow316
Copy link
Contributor Author

Yes, it looks great! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants