Skip to content

Conversation

@petyaslavova
Copy link
Collaborator

With this PR the following changes are introduced:

  • Adding oss_cluster_maint_notifications_handler initialization in the cluster client, that is transferred down to each connection's parser
  • Introducing new base class for cluster maintenance notifications - MaintNotificationsAbstractRedisCluster
  • Changing the available input arguments for the method that extracts cluster topology and updates the cached data - initialize
    • adding setting if the startup nodes should be disconnected after the topology is extracted - during the notification handling we have active connections that should be able to complete their in-progress tasks and allowing the nodes connection pools to disconnect will break all those commands executions, so when the topology is refreshed as part of SMIGRATED handling, the nodes are not disconnected, the old behavior is kept with the provided default value
    • adding one more additional parameter - host:port mapping for additional cluster nodes to be used during cluster topology extraction - when used as part of SMIGRATED handling, we provide the data received in the notification - it can be a new node that can be useful if the other nodes are not available.
  • Adding handling for the SMIGRATED event
    • It triggers cluster topology refresh
    • In case the node that holds the receiving connection is removed from the topology
      • all its free connections are disconnected
      • all in use connections are marked for disconnect and will be disconnected right after they complete their current command execution
      • no need to revert relaxed timeout
    • When the node stays in the topology(the notification is sent for partial slot migration) - the connections are not dissconnected
      • unrelax timeouts
  • Introducing fixes in the testing helper ProxyInterceptorHelper
  • Test coverage for the introduced changes (excluding pubsub coverage - this one will be added in separate PR)
Copy link
Contributor

Copilot AI left a 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 MaintNotificationsAbstractRedisCluster base class for cluster maintenance notification handling
  • Implements OSSMaintNotificationsHandler to 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.

redis/cluster.py Outdated

if maint_notifications_config and maint_notifications_config.enabled:
if not is_protocol_supported:
raise RedisError(
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants