Skip to content

Simplify RateLimiter to avoid handling expiration in code#10980

Closed
aduth wants to merge 10 commits intomainfrom
aduth-rm-rate-limiter-check-expired
Closed

Simplify RateLimiter to avoid handling expiration in code#10980
aduth wants to merge 10 commits intomainfrom
aduth-rm-rate-limiter-check-expired

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 24, 2024

🛠 Summary of changes

Updates RateLimiter to avoid handling expiration through code logic. Since we use Redis' built-in key expiration, we can infer expiration based on the absence of a key in the store.

Draft: Clarify assumptions.

📜 Testing Plan

TBD

aduth added 2 commits July 24, 2024 09:22
changelog: Internal, Rate Limiting, Simplify RateLimiter to avoid handling expiration in code
@aduth
Copy link
Contributor Author

aduth commented Jul 24, 2024

One potential issue here is that we rely on a local expiration value when incrementing multiple times on the same instance, to avoid calling to Redis (related failing spec).

We could keep some of the local expiration tracking for when incrementing while still removing the Redis GETEX operation, but I'm also wondering if this is really a scenario we encounter much in real usage, vs. contrived in specs. In typical usage, we're only ever incrementing once.

@aduth
Copy link
Contributor Author

aduth commented Jul 30, 2024

Going to close this for now until some of the other rate-limit related changes settle in this class, specifically those in #10982.

@aduth aduth closed this Jul 30, 2024
@aduth aduth deleted the aduth-rm-rate-limiter-check-expired branch July 30, 2024 13:18
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