Skip to content
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

Revert "New atomic-based implementation squeezed into int64" #91

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

rabbbit
Copy link
Contributor

@rabbbit rabbbit commented Jun 9, 2022

Reverts #85

See #90 - thanks @twelsh-aw for recording.

@rabbbit rabbbit merged commit 8b3fccf into main Jun 9, 2022
@rabbbit rabbbit deleted the revert-85-int64_rl branch June 9, 2022 18:25
rabbbit pushed a commit that referenced this pull request Jul 2, 2022
This limiter was introduced and merged in the following PR #85
Later @twelsh-aw found an issue with this implementation #90
So @rabbbit reverted this change in #91

Our tests did not detect this issue, so we have a separate PR #93 that enhances our tests approach to detect potential errors better.
With this PR, we want to restore the int64-based atomic rate limiter implementation as a non-default rate limiter and then check that #93 will detect the bug.
Right after it, we'll open a subsequent PR to fix this bug.
storozhukBM added a commit to storozhukBM/ratelimit that referenced this pull request Jul 2, 2022
This limiter was introduced and merged in the following PR uber-go#85
Later @twelsh-aw found an issue with this implementation uber-go#90
So @rabbbit reverted this change in uber-go#91

Our tests did not detect this issue, so we have a separate PR uber-go#93 that enhances our tests approach to detect potential errors better.
With this PR, we want to restore the int64-based atomic rate limiter implementation as a non-default rate limiter and then check that uber-go#93 will detect the bug.
Right after it, we'll open a subsequent PR to fix this bug.
rabbbit pushed a commit that referenced this pull request Oct 31, 2022
* Fix return timestamp discrepancy between regular atomic limiter and int64 based one
* Make int64 based atomic limiter default

Long story: this was added in #85, but reverted in #91 due to #90. #95 fixed the issue, so we're moving forward with the new implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant