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

Timeouts for redis client gotten via sentinel #1167

Merged
merged 1 commit into from
May 24, 2024

Conversation

jrylander
Copy link

Add a redis::sentinel::Sentinel::SentinelClient::get_async_connection_with_config that accepts timeouts

I have added just a happy test for calling with long timeouts. I am not versed enough to add a test for actual timeouts but am not sure it is needed since it just ships the parameters on to redis client.

redis/src/sentinel.rs Outdated Show resolved Hide resolved
@jrylander
Copy link
Author

Is this closer to your thinking? I am also guessing I should squash commits, right?

Copy link
Contributor

@nihohit nihohit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work, please fix & squash.

docsrs,
doc(cfg(any(feature = "tokio-comp", feature = "async-std-comp")))
)]
pub async fn get_multiplexed_async_connection_with_config(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_multiplexed_async_connection_with_timeouts should call this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Good thought

redis/src/sentinel.rs Outdated Show resolved Hide resolved
@nihohit
Copy link
Contributor

nihohit commented May 22, 2024

@jrylander looks like the change has some issues. Do you need help fixing this?

@jrylander
Copy link
Author

Ahhrg, my bad. I was on a clean computer and could not run tests. I am sorry!

Copy link
Contributor

@nihohit nihohit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. please fix the formatting issue blocking the CI.

@jrylander
Copy link
Author

Ran cargo fmt --all

@nihohit nihohit merged commit c010455 into redis-rs:main May 24, 2024
10 checks passed
nihohit added a commit to nihohit/redis-rs that referenced this pull request Jun 6, 2024
They're replaced with a constructor that uses a shared config type after redis-rs#1167
nihohit added a commit to nihohit/redis-rs that referenced this pull request Jun 7, 2024
They're replaced with a constructor that uses a shared config type after redis-rs#1167
nihohit added a commit to nihohit/redis-rs that referenced this pull request Jun 11, 2024
They're replaced with a constructor that uses a shared config type after redis-rs#1167
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.

None yet

2 participants