search

microlinkhq / async-ratelimiter

Rate limit made simple, easy, async.
MIT License
320 stars 23 forks source link

old data test and delete the old ones by score #5

Closed amanda94 closed 6 years ago

amanda94 commented 6 years ago

I add a test for my example and change the get function where every time accessing the sorted sets deleting the old data by score

Closes #3

Kikobeats commented 6 years ago

Thanks for that! I'm reviewing it and will be merge soon 🙂

Kikobeats commented 6 years ago

I want to know the opinion @xdmnl because I want to integrate his recent PR: https://github.com/tj/node-ratelimiter/pull/44

In your comment, you are suggesting use zremrangebyscore. Can both improvement life together? do you think that integrate your PR make necessary this one?

xdmnl commented 6 years ago

It is a trade-off between keeping all the data and do 2 reads or keeping only the maximum of timestamps requested by the limiter and then doing only 1 read. I like the first option (the one that I proposed) better because it reflects reality. If the delay is small enough, the difference won't be noticeable.

In any case, this PR is necessary. It fixes a bug that does not exist in the original library: not expiring out of date timestamps.

Kikobeats commented 6 years ago

Thanks for the feedback guys, released at 1.1.2