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

Fix retries in async mode #2180

Merged
merged 4 commits into from
Jun 19, 2022
Merged

Fix retries in async mode #2180

merged 4 commits into from
Jun 19, 2022

Conversation

elemoine
Copy link
Contributor

@elemoine elemoine commented May 12, 2022

Description of change

The main goal of this PR is to fix retries in asyncio mode. As described on #2179, in asyncio mode, there are currently no retries on other errors than TimeoutError errors.

To fix that bug the PR first makes the way retries are configured consistent in sync and async. It then modifies the _disconnect_raise function not to raise on any "supported error".

Note: I think there are other problems with the retry configuration API, but for now I just wanted to fix the actual bug while making the retry configuration API consistent between sync and async.

Fixes #2179.

Pull Request check-list

  • Does $ tox pass with this change (including linting)?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)? I have updated the async client's docstrings.
  • Is there an example added to the examples folder (if applicable)?
  • Was the change added to CHANGES file?

@elemoine elemoine changed the title Retry async Fix retries in async mode May 12, 2022
@elemoine elemoine force-pushed the retry-async branch 5 times, most recently from 0c1fb6b to 76f704d Compare May 13, 2022 08:59
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2022

Codecov Report

Merging #2180 (7944063) into master (42b937f) will increase coverage by 0.00%.
The diff coverage is 98.18%.

@@           Coverage Diff           @@
##           master    #2180   +/-   ##
=======================================
  Coverage   91.99%   91.99%           
=======================================
  Files         108      108           
  Lines       27393    27444   +51     
=======================================
+ Hits        25200    25248   +48     
- Misses       2193     2196    +3     
Impacted Files Coverage Δ
redis/asyncio/client.py 92.73% <80.00%> (-0.13%) ⬇️
redis/asyncio/connection.py 84.07% <100.00%> (+0.21%) ⬆️
redis/asyncio/retry.py 96.42% <100.00%> (+0.27%) ⬆️
redis/client.py 88.79% <100.00%> (+0.02%) ⬆️
redis/connection.py 86.42% <100.00%> (+0.06%) ⬆️
tests/test_asyncio/test_retry.py 100.00% <100.00%> (ø)
tests/test_asyncio/test_search.py 98.35% <0.00%> (-0.33%) ⬇️
tests/test_cluster.py 97.02% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42b937f...7944063. Read the comment docs.

@elemoine elemoine marked this pull request as ready for review May 13, 2022 10:01
@elemoine
Copy link
Contributor Author

This is ready for review.

@elemoine
Copy link
Contributor Author

elemoine commented Jun 7, 2022

Any chance that my PR gets some consideration? It fixes a real bug. Is there anything I should do?

@dvora-h
Copy link
Collaborator

dvora-h commented Jun 19, 2022

@elemoine Thank you for the PR and for fixing the bug you reported.! Merging, and ensuring this is part of the 4.3.4 release.

@dvora-h dvora-h merged commit bea7299 into redis:master Jun 19, 2022
@elemoine elemoine deleted the retry-async branch June 20, 2022 06:57
@elemoine
Copy link
Contributor Author

@dvora-h thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retries do no work with asyncio
3 participants