Skip to content

Fix cluster pipeline retries for connection timeouts#284

Open
mkmkme wants to merge 3 commits into
mainfrom
mkmkme/cluster-timeout
Open

Fix cluster pipeline retries for connection timeouts#284
mkmkme wants to merge 3 commits into
mainfrom
mkmkme/cluster-timeout

Conversation

@mkmkme

@mkmkme mkmkme commented Mar 21, 2026

Copy link
Copy Markdown
Collaborator

Reuse the shared cluster reinitialize error set so pipeline connection checkout timeouts refresh topology and retry like single commands. Add sync regression coverage for get_connection() timeouts and an asyncio guard test for existing retry behavior.

Fixes #261

Pull Request check-list

  • Do tests and lints pass with this change?
  • 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)?
  • Is there an example added to the examples folder (if applicable)?

Description of change

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR aligns cluster pipeline retry behavior with single-command execution so that connection checkout timeouts trigger topology refresh + retry, and adds regression coverage for both sync and asyncio clients.

Changes:

  • Introduces a shared REINITIALIZE_ERRORS tuple on AbstractValkeyCluster and reuses it in ClusterPipeline.
  • Updates ClusterPipeline.send_cluster_commands() / _send_cluster_commands() to reinitialize and retry on connection checkout TimeoutError (and other reinit-worthy errors).
  • Adds sync regression tests for get_connection() timeout retry/raise behavior and an asyncio guard test for timeout retry behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
valkey/cluster.py Unifies retryable reinit errors and extends pipeline retry/reinitialize behavior to include connection checkout timeouts.
tests/test_cluster.py Adds sync regression tests covering timeout retry and exhausted-retry behavior for pipeline connection acquisition.
tests/test_asyncio/test_cluster.py Adds asyncio test ensuring pipeline retries on TimeoutError during execution.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread valkey/cluster.py Outdated
Comment thread valkey/cluster.py
@codecov-commenter

codecov-commenter commented Mar 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.45%. Comparing base (98d3b5c) to head (134016b).
⚠️ Report is 89 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #284      +/-   ##
==========================================
+ Coverage   76.40%   76.45%   +0.05%     
==========================================
  Files         129      129              
  Lines       34065    34125      +60     
==========================================
+ Hits        26027    26090      +63     
+ Misses       8038     8035       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread tests/test_asyncio/test_cluster.py Outdated
Comment thread tests/test_cluster.py Outdated
Comment thread tests/test_cluster.py Outdated
@mkmkme

mkmkme commented Mar 28, 2026

Copy link
Copy Markdown
Collaborator Author

@copilot review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread valkey/cluster.py
Comment thread valkey/cluster.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mkmkme added 3 commits March 28, 2026 11:29
Reuse the shared cluster reinitialize error set so pipeline connection
checkout timeouts refresh topology and retry like single commands. Add
sync regression coverage for get_connection() timeouts and an asyncio
guard test for existing retry behavior.

Fixes #261

Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
@mkmkme mkmkme force-pushed the mkmkme/cluster-timeout branch from e029e22 to 134016b Compare March 28, 2026 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

Checking TimeoutError from get_connection in _execute_cluster_command

4 participants