throttle: add support for Redis installs without redis-cell module #282
Conversation
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
This hasn't seen any testing, just pushing up my current state for possible feedback.
djc
force-pushed
the
redis-gcra
branch
2 times, most recently
from
918c6a5 to
1bab348
Compare
djc
force-pushed
the
redis-gcra
branch
2 times, most recently
from
1cce908 to
fc95611
Compare
|
@wez the new test now passes which leads me to think this is probably done? Let me know if you want to see more test coverage. |
crates/throttle/src/throttle.rs
Outdated
| remaining: result.1, | ||
| reset_after: Duration::from_secs(result.2), | ||
| retry_after: match result.3 { | ||
| 0 => None, |
There was a problem hiding this comment.
the redis_cell_throttle implementation of this above handles the case where the number is negative, and also has a different representation for 0.
Which is right? TBH, I think neither is quite right!
The logic that consumes the throttles assumes that if retry_after.is_some(), then we were throttled, and will break out of the throttle checking loop otherwise. It's probably best to formally encode that here for the various implementations and ensure that this is None if we are not throttled, and otherwise set this to the returned interval, clamping negative values to 0 so that we can't panic or return overflown/wrapped unsigned durations for this case.
I confess to not knowing whether it is possible to get a 0 second result here if we are throttled. That might be an awkward edge case that produces some busier looping in the consumer, so I'm open to considering clamping to a minimum of 1 second in the throttled case.
What do you think?
There was a problem hiding this comment.
Hmm, I noticed that ThrottleResult::retry_after is actually the only field from that type that gets used outside the crate. Should we just get rid of everything else?
Will dig more into the details of this tomorrow, my brain is a little too fried to go deep on this now.
There was a problem hiding this comment.
preserving the other fields seems useful to me, if for example, we wanted to have those automatically translate into the conventional set of http throttling headers in the response to an http request.
There was a problem hiding this comment.
For the script throttle, retry_after has math.ceil() applied to it, so I don't think it can return 0 when throttling happens (in that case diff must be non-zero because remaining is not 0). I think that also means it's always more than 0 in the throttled case. I've made the code a bit more similar to the redis_cell_throttle().
Not sure what else to do here? Feel free to make changes to the PR if you just want to get it merged.
djc
force-pushed
the
redis-gcra
branch
2 times, most recently
from
b5e5b76 to
5fa5a6d
Compare
wez
left a comment
wez
left a comment
There was a problem hiding this comment.
Looks like there's an issue with running make test
crates/throttle/src/lib.rs
Outdated
| pub struct ThrottleResult { | ||
| /// true if the action was limited | ||
| pub throttled: bool, | ||
| pub(crate) throttled: bool, |
There was a problem hiding this comment.
I'd like to keep these in the public interface for future use
You mean this, right? 
This confuses since I have a commit that should address this ("mod-redis: derive Debug for RedisConnection"). |
|
looks like some kind of weird issue with the CI; pulling and building it myself passes the tests just fine, so let's merge this! |
2 participants
Uses a Lua script for GCRA from https://github.com/Losant/redis-gcra/blob/master/lib/gcra.lua to perform the necessary logic without support for the redis-cell module that provides
CL.THROTTLE.Fixes #275.