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

Recompute keys in atomic incr with sliding window #69

Merged
merged 3 commits into from Apr 17, 2018

Conversation

@gdvalle
Copy link
Contributor

@gdvalle gdvalle commented Apr 15, 2018

Change the WindowRateLimiter to pass a callable to incr_and_sum so
that during atomic increments the time windows can be recomputed in the
event of a lock.

This breaks the API because we change incr_and_sum's keys arg to
a callable. We keep backwards compatibility by checking if keys is
callable and calling it on each use.

@@ -100,6 +100,8 @@ def incr_and_sum(self, key, keys, amount, maximum, ttl):
if value > maximum:
return False

# TODO: Drop non-callable keys in Dramatiq v2.
keys = keys() if callable(keys) else keys

This comment has been minimized.

@Bogdanp

Bogdanp Apr 15, 2018
Owner

This should be renamed to key_list or something since it's in a loop to avoid shadowing the keys callable.

gdvalle added 2 commits Apr 16, 2018
Change the `WindowRateLimiter` to pass a callable to `incr_and_sum` so
that during atomic increments the time windows can be recomputed in the
event of a lock.

This breaks the API because we change `incr_and_sum`'s `keys` arg to
a callable. We keep backwards compatibility by checking if `keys` is
callable and calling it on each use.
@gdvalle gdvalle force-pushed the gdvalle:ratelimit-keys branch from b68c4af to bbc3299 Apr 16, 2018
@Bogdanp Bogdanp merged commit 8528e4c into Bogdanp:master Apr 17, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate 4 issues to fix
Details
@gdvalle gdvalle deleted the gdvalle:ratelimit-keys branch Apr 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants