- Notifications
You must be signed in to change notification settings - Fork 2.6k
Adding handling of SMIGRATED push notifications #3857
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
base: feat/hitless_upgrade_sync_cluster_client
Are you sure you want to change the base?
Adding handling of SMIGRATED push notifications #3857
Conversation
There was a problem hiding this comment.
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 introduces support for handling SMIGRATED push notifications in Redis cluster clients, enabling automatic cluster topology updates during slot migrations.
Key Changes
- Introduces
MaintNotificationsAbstractRedisClusterbase class for cluster maintenance notification handling - Implements
OSSMaintNotificationsHandlerto process SMIGRATED notifications and refresh cluster topology - Updates
NodesManager.initialize()to support additional startup nodes and configurable connection pool disconnection behavior during topology refresh
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
redis/cluster.py | Adds MaintNotificationsAbstractRedisCluster abstract class and integrates OSS cluster notification handler into RedisCluster |
redis/maint_notifications.py | Implements OSSMaintNotificationsHandler with SMIGRATED notification processing and connection pool management |
redis/connection.py | Extends connection initialization to support OSS cluster maintenance handlers and updates configuration propagation |
redis/client.py | Adds oss_cluster_maint_notifications_handler parameter to Redis client constructor |
redis/_parsers/base.py | Updates notification parsing to use safe_str() for consistent string handling |
tests/maint_notifications/test_maint_notifications.py | Updates tests for OSSNodeMigratedNotification to require node_address parameter |
tests/maint_notifications/test_cluster_maint_notifications_handling.py | Adds comprehensive test coverage for SMIGRATING/SMIGRATED notification handling scenarios |
tests/maint_notifications/proxy_server_helpers.py | Refactors proxy interceptor helper to support flexible cluster slot configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3c211ef to 76172f9 Compare redis/cluster.py Outdated
| | ||
| if maint_notifications_config and maint_notifications_config.enabled: | ||
| if not is_protocol_supported: | ||
| raise RedisError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to throw it as soon as protocol check happens. If it's not supported self.maint_notifications_config will anyway be None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
With this PR the following changes are introduced: