feat(redis): Add response timeout #5329
Open
Conversation
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
loewenheim
commented
Nov 3, 2025
Comment on lines
-37
to
-40
|
|
||
| /// An error that occurs when configuring Redis. | ||
| #[error("failed to configure redis: {0}")] | ||
| ConfigError(#[from] ConfigError), |
There was a problem hiding this comment.
This variant and From impl were already unused.
loewenheim
commented
Nov 4, 2025
relay-redis/src/config.rs
Outdated
| /// will fail with a timeout error. This setting helps prevent indefinite | ||
| /// blocking when the pool is exhausted. | ||
| pub wait_timeout: Option<u64>, | ||
| pub connection_timeout: Option<u64>, |
There was a problem hiding this comment.
This is arguably redundant with create_timeout: This timeout is used during connection creation in the redis crate, whereas create_timeout is used in the Managers' implementations of create. Both have the effect of putting a time limit on establishing a connection, but one is at the redis level and one at the deadpool level.
|
Updates:
|
loewenheim
changed the title
tobias-wilfert
approved these changes
Nov 4, 2025
This adds connection and respnose timeout config options for both Single and Cluster Redis clients. Unfortunately, since `redis` and `deadpool-redis` don't provide enough knobs to tweak the default clients, the easiest way to accomplish this is by more or less reinventing `deadpool_redis::Manager` and `deadpool_redis::Cluster::Manager`, but with timeouts applied. Consequently the `deadpool_redis` dependency becomes unused. TBD: default values and docstrings for the new timeouts.
loewenheim
force-pushed
the
sebastian/redis-timeouts
branch
from
5a27321 to
947d4dc
Compare
Dav1dde
reviewed
Nov 4, 2025
| create_timeout: Some(3), | ||
| recycle_timeout: Some(2), | ||
| wait_timeout: None, | ||
| response_timeout: Some(6), |
There was a problem hiding this comment.
This seems very low for the total command time, how did you arrive at this number?
I suspect that some cardinality limiting commands take longer than this.
| /// An error leads to the connection being detached if it is either unrecoverable or a timeout. | ||
| fn should_be_detached(error: &RedisError) -> bool { | ||
| error.is_unrecoverable_error() | ||
| error.is_unrecoverable_error() || error.is_timeout() |
There was a problem hiding this comment.
Could add a small justification on to why the timeout is listed here.
Comment on lines
+211
to
+213
| Err(deadpool::managed::RecycleError::message( | ||
| "Invalid PING response", | ||
| )) |
There was a problem hiding this comment.
Suggested change
| Err(deadpool::managed::RecycleError::message( | |
| "Invalid PING response", | |
| )) | |
| Err(RecycleError::message("Invalid PING response")) |
RecycleError is already imported.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This adds connection and respnose timeout config options for both Single and Cluster Redis clients.
Unfortunately, since
redisanddeadpool-redisdon't provide enough knobs to tweak the default clients, the easiest way to accomplish this is by more or less reinventingdeadpool_redis::Manageranddeadpool_redis::Cluster::Manager, but with timeouts applied. Consequently thedeadpool_redisdependency becomes unused.TBD: default values and docstrings for the new timeouts.
Fixes INGEST-621.