RedisClient: add redis_flakey mechanism to avoid repeated timeouts#1361
Merged
Conversation
risicle
force-pushed
the
ris-redis-client-flakey-flag
branch
from
3464ba8 to
ff1013c
Compare
robinjam
approved these changes
Jun 16, 2026
robinjam
left a comment
robinjam
left a comment
Contributor
There was a problem hiding this comment.
I think I'm happy with this approach. Really sorry it took so long to get reviewed...
Happy to re-approve once you've fixed the merge conflicts
Member
Author
|
It's no problem - it had to have proper consideration. |
risicle
force-pushed
the
ris-redis-client-flakey-flag
branch
from
ff1013c to
97bd626
Compare
when connections to redis are timing out, though we can reduce the timeout wait, many requests will attempt to access redis on multiple occasions. in these cases it will end up wating the timeout period each time. these can add up to make a request fail (e.g. run into an `EventletTimeout`) where it could otherwise have passed if it had got the hint that redis was in bad shape the first time it got a timeout. this adds an app-context-local flag `redis_flakey` that will prevent repeated attempts at accessing redis for calls that are deemed `skippable`. `skippable` means that the consequences of omitting the redis request are not catastrophic and won't cause a consistency problem (just slightly poorer performance). call sites can themselves set the `skippable` argument, but by default `get`s are considered skippable and `set`s aren't. `RequestCache.set` overrides this to set `skippable=True` for its `RedisClient.set` calls because the inner `client_method` call *should* be side-effect-free - i.e. nothing has just happened that requires us to invalidate an existing cache entry.
risicle
force-pushed
the
ris-redis-client-flakey-flag
branch
from
97bd626 to
9dd094e
Compare
quis
approved these changes
Jun 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://trello.com/c/PEpdGAPE/1518-implement-solution-for-failed-writes-deletes-to-redis
When connections to redis are timing out, though we can reduce the timeout wait, many requests will attempt to access redis on multiple occasions. in these cases it will end up waiting the timeout period each time. These can add up to make a request fail (e.g. run into an
EventletTimeout) where it could otherwise have passed if it had got the hint that redis was in bad shape the first time it got a timeout.This adds an app-context-local flag
redis_flakeythat will prevent repeated attempts at accessing redis for calls thatare deemed
skippable.skippablemeans that the consequences of omitting the redis request are not catastrophic and won't cause a consistency problem (just slightly poorer performance).Call sites can themselves set the
skippableargument, but by defaultgets are considered skippable andsets aren't.RequestCache.setoverrides this to setskippable=Truefor itsRedisClient.setcalls because the innerclient_methodcall should be side-effect-free - i.e. nothing has just happened that requires us to invalidate an existing cache entry.