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

Optimize increment and decrement for RedisCacheStore #45711

Merged
merged 1 commit into from Aug 1, 2022

Conversation

fatkodima
Copy link
Member

@fatkodima fatkodima commented Jul 30, 2022

Before this PR, when incrementing/decrementing a counter with :expires_in option, we will make 3 requests to Redis.

increment/decrement, for example, is very heavily used by rack-attack - https://github.com/rack/rack-attack/blob/95ce9fdd7c99a527a46ffc477b01e682fed48dce/lib/rack/attack/store_proxy/redis_cache_store_proxy.rb#L13-L25 (that implementation currently also makes redundant calls and works around unsupported expires_in in older versions; the implementation should just use the native implementation from rails in the future). Increment can be called several times per request (for each throttling rule).

With this PR, we will make only 1 pipelined request, if possible.

cc @byroot

@casperisfine
Copy link
Contributor

@fatkodima thanks for the PR, and I'm in favor of this improvement, however I'm afraid there's a bit too much unrelated and not strictly necessary changes. Not saying they're not desirable, but it would make it much easier if this PR was really only focused on the pipelining change.

The state of the code makes me think we should really refactor all this so that Redis::Distributed / Redis::Cluster support doesn't make the code as unwidely, but I think it's best done in a dedicated PR.

@fatkodima fatkodima force-pushed the redis-cache-store-optimize-incr-decr branch from 2a5f98f to 8673130 Compare August 1, 2022 09:49
@fatkodima
Copy link
Member Author

@byroot Thanks for the review. Agreed. That changes were left from my original implementation, now they are not needed.
Updated to include only the relevant changes.

@fatkodima fatkodima force-pushed the redis-cache-store-optimize-incr-decr branch from 8673130 to 1ceb596 Compare August 1, 2022 10:23
@fatkodima fatkodima force-pushed the redis-cache-store-optimize-incr-decr branch from 1ceb596 to 412f137 Compare August 1, 2022 14:08
@fatkodima
Copy link
Member Author

@byroot Rebased on top of the refactoring with the #45711 (comment) suggestion. Let me know if some other changes needed.

@fatkodima fatkodima force-pushed the redis-cache-store-optimize-incr-decr branch from 412f137 to e35eb0f Compare August 1, 2022 14:23
@byroot byroot merged commit 26aafc3 into rails:main Aug 1, 2022
@fatkodima fatkodima deleted the redis-cache-store-optimize-incr-decr branch August 1, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants