PYTHON-539 Timeouts in ResponseFuture should notify the conviction policy #1256
Conversation
…when calling Cluster.signal_connection_failure (which we do in the unit test).
There was a problem hiding this comment.
Pull request overview
This PR implements a fix for PYTHON-539 by ensuring that timeouts in ResponseFuture notify the host's conviction policy. This allows the driver to take action on repeated or systemic timeouts through the conviction policy mechanism.
Key Changes:
- Modified
ResponseFuture._on_timeout()to callsession.cluster.signal_connection_failure()when a timeout occurs - Added comprehensive test coverage for the timeout-to-conviction-policy integration
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit/test_response_future.py | Added test case to verify timeouts trigger conviction policy notifications |
| cassandra/cluster.py | Updated timeout handling to signal connection failures to the conviction policy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # An extra bit of connective tissue. session.cluster is a mock but we want to use the | ||
| # actual impl in Cluster in order to get into the host (and from there to the conviction | ||
| # policy). As of this writing Cluster.signal_connection_failure is effectively static | ||
| # if the return value from add_failure() on the conviction poilcy is false so we can |
There was a problem hiding this comment.
Corrected spelling of 'poilcy' to 'policy'.
| # if the return value from add_failure() on the conviction poilcy is false so we can | |
| # if the return value from add_failure() on the conviction policy is false so we can |
| def foo(*args, **kwargs): | ||
| Cluster.signal_connection_failure(Cluster(), *args, **kwargs) | ||
| session.cluster.signal_connection_failure.side_effect = foo |
There was a problem hiding this comment.
The function name 'foo' is not descriptive. Consider renaming it to something more meaningful like 'signal_failure_wrapper' or 'delegate_signal_connection_failure' to clarify its purpose of delegating to the actual Cluster implementation.
| def foo(*args, **kwargs): | |
| Cluster.signal_connection_failure(Cluster(), *args, **kwargs) | |
| session.cluster.signal_connection_failure.side_effect = foo | |
| def delegate_signal_connection_failure(*args, **kwargs): | |
| Cluster.signal_connection_failure(Cluster(), *args, **kwargs) | |
| session.cluster.signal_connection_failure.side_effect = delegate_signal_connection_failure |
1 participant
Replacement for #1127