-
Notifications
You must be signed in to change notification settings - Fork 300
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
Make int64 based atomic ratelimiter default #101
Make int64 based atomic ratelimiter default #101
Conversation
Codecov Report
@@ Coverage Diff @@
## main #101 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 95 98 +3
=========================================
+ Hits 95 98 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
sleepDuration := time.Duration(newTimeOfNextPermissionIssue - now) | ||
if sleepDuration > 0 { | ||
t.clock.Sleep(sleepDuration) | ||
return time.Unix(0, newTimeOfNextPermissionIssue) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, now I'm confused again :)
- you removed very similar lines in Fix no slack option for int64 based option #95 (https://github.com/uber-go/ratelimit/pull/95/files#diff-30af80b2f0055e05bdc7d6ecea1b49d55f1ef946c381a1604f8e03d58789cd3fL85)
- Fix test approach for detecting issues #93 which "fixes the time" - which I assumed is the goal-state - doesn't have this change.
Why re-introduce this now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 month ago, you added a new test Example_default
that checks the slack behaviour of the default ratelimiter.
It turns out, atomicLimiter
returns now
if it doesn't sleep, so I decided to sync this behaviour in atomicInt64Limiter
.
you removed very similar lines in #95
Yes, these lines are similar, but slightly different in the current PR we return now
if we don't sleep, and this is the only behaviour change, everything else works as it worked before.
#93 which "fixes the time" - which I assumed is the goal-state - doesn't have this change.
In #93 atomicLimiter
is still a default implementation, so I didn't know about the difference between behaviours so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rabbbit ^^
No description provided.