Replace default health check and failure detector with custom by vladvildanov · Pull Request #3822 · redis/redis-py (original) (raw)
Pull Request check-list
Please make sure to review and check all of these items:
- 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)?
NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
Please provide a description of the change here.
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 refactors how health checks and failure detectors are configured in the MultiDBClient. Instead of extending the default checks/detectors with user-provided ones, the code now replaces the defaults entirely when custom ones are provided. This simplifies the initialization logic and gives users more control over which checks/detectors to use.
Key Changes:
- Changed health checks and failure detectors initialization from an extending pattern to a replacement pattern
- Replaced
patch.objectwith direct attribute assignment in tests to match the new configuration approach - Updated documentation to clarify that users can either provide their own set or add to the defaults
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| redis/multidb/client.py | Simplified health checks and failure detectors initialization to use replacement pattern instead of extending defaults |
| redis/asyncio/multidb/client.py | Same refactoring as sync version for async MultiDBClient |
| tests/test_multidb/test_client.py | Updated tests to set health_checks directly on mock config instead of patching default_health_checks method |
| tests/test_asyncio/test_multidb/test_client.py | Same test updates as sync version for async tests |
| docs/multi_database.rst | Clarified that users can provide custom health checks/failure detectors or add to defaults |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines +32 to +36
| self._health_checks = ( |
|---|
| config.default_health_checks() |
| if not config.health_checks |
| else config.health_checks |
| ) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change breaks backward compatibility. Previously, custom health_checks were added to the defaults via extend(), allowing users to augment default behavior. Now they completely replace defaults. Users who expected to add additional checks alongside the default PingHealthCheck will lose the default check. Consider providing a clear migration path or maintaining the extending behavior.
| self._health_checks = ( |
|---|
| config.default_health_checks() |
| if not config.health_checks |
| else config.health_checks |
| ) |
| if config.health_checks: |
| self._health_checks = config.default_health_checks() + config.health_checks |
| else: |
| self._health_checks = config.default_health_checks() |
Copilot uses AI. Check for mistakes.
Comment on lines +41 to +45
| self._failure_detectors = ( |
|---|
| config.default_failure_detectors() |
| if not config.failure_detectors |
| else config.failure_detectors |
| ) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change breaks backward compatibility. Previously, custom failure_detectors were added to the defaults via extend(), allowing users to augment default behavior. Now they completely replace defaults. Users who expected to add additional detectors alongside the default CommandFailureDetector will lose the default detector. Consider providing a clear migration path or maintaining the extending behavior.
| self._failure_detectors = ( |
|---|
| config.default_failure_detectors() |
| if not config.failure_detectors |
| else config.failure_detectors |
| ) |
| self._failure_detectors = config.default_failure_detectors() |
| if config.failure_detectors: |
| self._failure_detectors.extend(config.failure_detectors) |
Copilot uses AI. Check for mistakes.
Comment on lines +31 to +35
| self._health_checks = ( |
|---|
| config.default_health_checks() |
| if not config.health_checks |
| else config.health_checks |
| ) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change breaks backward compatibility. Previously, custom health_checks were added to the defaults via extend(), allowing users to augment default behavior. Now they completely replace defaults. Users who expected to add additional checks alongside the default PingHealthCheck will lose the default check. Consider providing a clear migration path or maintaining the extending behavior.
| self._health_checks = ( |
|---|
| config.default_health_checks() |
| if not config.health_checks |
| else config.health_checks |
| ) |
| if config.health_checks: |
| self._health_checks = config.default_health_checks() + config.health_checks |
| else: |
| self._health_checks = config.default_health_checks() |
Copilot uses AI. Check for mistakes.
Comment on lines +41 to +45
| self._failure_detectors = ( |
|---|
| config.default_failure_detectors() |
| if not config.failure_detectors |
| else config.failure_detectors |
| ) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change breaks backward compatibility. Previously, custom failure_detectors were added to the defaults via extend(), allowing users to augment default behavior. Now they completely replace defaults. Users who expected to add additional detectors alongside the default CommandFailureDetector will lose the default detector. Consider providing a clear migration path or maintaining the extending behavior.
| self._failure_detectors = ( |
|---|
| config.default_failure_detectors() |
| if not config.failure_detectors |
| else config.failure_detectors |
| ) |
| self._failure_detectors = config.default_failure_detectors() |
| if config.failure_detectors: |
| self._failure_detectors.extend(config.failure_detectors) |
Copilot uses AI. Check for mistakes.
