enova / sidekiq-rate-limiter

Redis-backed, per-worker rate limits for job processing.
http://rubygems.org/gems/sidekiq-rate-limiter
MIT License
193 stars 43 forks source link

Fix interval bug in fetch.rb #22

Closed hwayne closed 7 years ago

hwayne commented 8 years ago

On line #40 limit uses redis_rate_limiter to call EXPIRE with interval as a parameter. Since EXPIRE only accepts integers, this causes lim.add to fail. Since redis_rate_limiter use a multi block, lim.add fails silently instead of raising an error:

image

This means that nothing is added to the rate limit queue, so no jobs are actually rate limited. This PR fixes this bug by casting to an integer instead of a float.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 43ebb12662a5c11dcb7f4a2d37dff240f1a754f3 on hwayne:patch-1 into 1f870129309d0ba7cbcf0977bdb590ce7ae5bf0e on enova:master.

mlarraz commented 8 years ago

👍

Can you add a short test for this please?

hwayne commented 8 years ago

More testing shows that redis does not do rollbacks, so this bug only means that the queue won't expire naturally (an optimization). Will provide tests, but this might be on the backburner for me for a few days, sorry :(