Replace default health check and failure detector with custom #3822
Conversation
There was a problem hiding this comment.
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.
| self._health_checks = ( | ||
| config.default_health_checks() | ||
| if not config.health_checks | ||
| else config.health_checks | ||
| ) |
There was a problem hiding this comment.
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() |
| self._failure_detectors = ( | ||
| config.default_failure_detectors() | ||
| if not config.failure_detectors | ||
| else config.failure_detectors | ||
| ) |
There was a problem hiding this comment.
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) |
| self._health_checks = ( | ||
| config.default_health_checks() | ||
| if not config.health_checks | ||
| else config.health_checks | ||
| ) |
There was a problem hiding this comment.
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() |
| self._failure_detectors = ( | ||
| config.default_failure_detectors() | ||
| if not config.failure_detectors | ||
| else config.failure_detectors | ||
| ) |
There was a problem hiding this comment.
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) |
Pull Request check-list
Please make sure to review and check all of these items:
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.