Tmp #1
Tmp #1
Conversation
There was a problem hiding this comment.
I added a few comments. I tried to remove the issue with the minimum delay of 1 ms, but I gave up after making the tests hang a couple of times. Property-based tests are very difficult to trace and debug. I wonder how an equivalent suite with just regular tests would look like, because the tested code is simple (apart from the rounding) and the tests look like the most complex part of the whole PR.
| @@ -0,0 +1,10 @@ | |||
| -ifndef(OPUNTIA). | |||
There was a problem hiding this comment.
I wouldn't because then users of this library would do
-include_lib("opuntia/include/opuntia.hrl")And I don't want them to look at the internals of the record 🤔
There was a problem hiding this comment.
Oh... ok. I guess it's justified then. My Erlang LS is just confused where is the record definition.
| PossibleTokenGrowth = round(MaxRatePerMs * TimeSinceLastUpdate), | ||
| % Available plus recovered cannot grow higher than the actual rate limit | ||
| ExactlyAvailableNow = min(MaxRatePerMs, LastAvailableTokens + PossibleTokenGrowth), | ||
| % Now check how many tokens are available by substracting how many where used, |
There was a problem hiding this comment.
| % Now check how many tokens are available by substracting how many where used, | |
| % Now check how many tokens are available by subtracting how many where used, |
| TokensAvailable = max(0, ExactlyAvailableNow - TokensNowUsed), | ||
| TokensOverused = max(0, TokensNowUsed - ExactlyAvailableNow), | ||
| MaybeDelay = TokensOverused / MaxRatePerMs, | ||
| RoundedDelay = floor(MaybeDelay) + 1, |
There was a problem hiding this comment.
This makes the shaper wait 1 ms even if TokensNowUsed is zero, which is not correct IMO. I think that if wait is called very often, we would artificially slow down even if it shouldn't be the case.
| MaybeDelay = TokensOverused / MaxRatePerMs, | ||
| RoundedDelay = floor(MaybeDelay) + 1, | ||
| NewShaper = Shaper#token_bucket{available_tokens = TokensAvailable, | ||
| last_update = Now + MaybeDelay}, |
There was a problem hiding this comment.
Something doesn't quite add up here for me, because the returned delay is different from the one added to Now. I haven't done the exact math, from the top of my head we are skewing the shaper towards slowing down traffic too much. IMO we should keep track of the exact wait times returned - otherwise it gets out of sync. The additional round in line 42 makes it harder to calculate in my head.
There was a problem hiding this comment.
The point is that in the bucket I want to keep track of the exact times without rounding errors, so if in this case I asked to wait 0.7ms more than I should have, the bucket knows this and in the next request it will request to wait 0.3ms less, compensating.
But in the returned delay I wanted to give integers and not floats so that they can be given to the corresponding timeout directly.
There was a problem hiding this comment.
Ok, it all depends on what we do with the +1 in line. If we replace it with a fair rounding (not always adding a systematic error), your assumptions will be ok. The only thing I would point out is that we are losing precision here, because float is only very accurate around zero, while timestamps are very far from zero. I tested it and we still have 4 digits of precision:
36> Now = erlang:monotonic_time(millisecond).
-576458575501
37> float(Now) + 0.00001.
-576458575501.0
38> float(Now) + 0.0001.
-576458575500.9999
I don't have a smart solution, but my intuition tells me that storing the error separately and adjusting for it in the returned values (e.g. return at least Delay + Correction) might be worth considering.
| -record(opuntia_state, { | ||
| name :: name(), | ||
| max_delay :: opuntia:delay(), %% Maximum amount of time units to wait | ||
| gc_ttl :: non_neg_integer(), %% How many seconds to store each shaper |
There was a problem hiding this comment.
I wouldn't call it GC because it automatically makes me think of Erlang GC. I'd just use the term cleanup everywhere.
| [TokensToSpend, RatePerMs, TimeMs, MinimumExpected, Val]), | ||
| Val | ||
| end), | ||
| run_prop(?FUNCTION_NAME, Prop, 100_000, 256). |
There was a problem hiding this comment.
Do we really need such high number of tests? The properties are pretty simple...
No description provided.