Applying review comments. Finilized the notifications format.

Author: petyaslavova

redis/_parsers/base.py

@@ -184,38 +184,18 @@ class MaintenanceNotificationsParser:
     @staticmethod
     def parse_oss_maintenance_start_msg(response):
         # Expected message format is:
-        # TODO Clarify which format will be the final one
         # SMIGRATING <seq_number> <slot, range1-range2,...>
-        # or
-        # (using the one below for testing until the server changes are done)
-        # SMIGRATING <transaction_id> TO <endpoint> <list of slots and slot-ranges>  ## received on source endpoint
-        # SMIGRATING <transaction_id> FROM <list of slots and slot-ranges>.      ## received on target endpoint
         id = response[1]
-
-        address_value = response[3]
-        if isinstance(address_value, bytes):
-            address_value = address_value.decode()
-        if response[2] in ("TO", b"TO"):
-            dest_node = address_value
-            src_node = None
-        else:
-            dest_node = None
-            src_node = address_value
-
-        slots = response[4]
-        return OSSNodeMigratingNotification(id, src_node, dest_node, slots)
+        slots = response[2]
+        return OSSNodeMigratingNotification(id, slots)
 
     @staticmethod
     def parse_oss_maintenance_completed_msg(response):
         # Expected message format is:
-        # TODO Clarify which format will be the final one
-        # SMIGRATED <seq_number> NodeA’<host:port> <slot, range1-range2,...>
-        # or
-        # SMIGRATED <transaction_id> <list of slots and slot-ranges>
-        # received on source and target endpoints when migration is over
+        # SMIGRATED <seq_number> <host:port> <slot, range1-range2,...>
         id = response[1]
-        node_address = None
-        slots = response[2]
+        node_address = response[2]
+        slots = response[3]
         return OSSNodeMigratedNotification(id, node_address, slots)
 
     @staticmethod
@@ -453,18 +433,16 @@ async def handle_push_response(self, response, **kwargs):
 
                 if notification is not None:
                     return await self.maintenance_push_handler_func(notification)
-                if (
-                    msg_type == _SMIGRATED_MESSAGE
-                    and self.oss_cluster_maint_push_handler_func
-                ):
-                    parser_function = MSG_TYPE_TO_MAINT_NOTIFICATION_PARSER_MAPPING[
-                        msg_type
-                    ][1]
-                    notification = parser_function(response)
-                    if notification is not None:
-                        return await self.oss_cluster_maint_push_handler_func(
-                            notification
-                        )
+            if (
+                msg_type == _SMIGRATED_MESSAGE
+                and self.oss_cluster_maint_push_handler_func
+            ):
+                parser_function = MSG_TYPE_TO_MAINT_NOTIFICATION_PARSER_MAPPING[
+                    msg_type
+                ][1]
+                notification = parser_function(response)
+                if notification is not None:
+                    return await self.oss_cluster_maint_push_handler_func(notification)
         except Exception as e:
             logger.error(
                 "Error handling {} message ({}): {}".format(msg_type, response, e)

redis/maint_notifications.py

@@ -406,12 +406,6 @@ class OSSNodeMigratingNotification(MaintenanceNotification):
 
     Args:
         id (int): Unique identifier for this notification
-        src_node (Optional[str]): Source node address - the notifications
-                                received by the connections to the src node will
-                                receive the dest node address
-        dest_node (Optional[str]): Destination node address - the notifications
-                                received by the connections to the dst node will
-                                receive the src node address
         slots (Optional[List[int]]): List of slots being migrated
     """
 
@@ -420,23 +414,17 @@ class OSSNodeMigratingNotification(MaintenanceNotification):
     def __init__(
         self,
         id: int,
-        src_node: Optional[str] = None,
-        dest_node: Optional[str] = None,
         slots: Optional[List[int]] = None,
     ):
         super().__init__(id, OSSNodeMigratingNotification.DEFAULT_TTL)
         self.slots = slots
-        self.src_node = src_node
-        self.dest_node = dest_node
 
     def __repr__(self) -> str:
         expiry_time = self.creation_time + self.ttl
         remaining = max(0, expiry_time - time.monotonic())
         return (
             f"{self.__class__.__name__}("
             f"id={self.id}, "
-            f"src_node={self.src_node}, "
-            f"dest_node={self.dest_node}, "
             f"slots={self.slots}, "
             f"ttl={self.ttl}, "
             f"creation_time={self.creation_time}, "

tests/maint_notifications/test_cluster_maint_notifications_handling.py

@@ -322,7 +322,7 @@ def _get_expected_node_state(
     ) -> Optional[ConnectionStateExpectation]:
         """Get the expected state for a node."""
         for expectation in expectations_list:
-            if int(expectation.node_port) == (node_port):
+            if expectation.node_port == node_port:
                 return expectation
         return None
 
@@ -397,7 +397,7 @@ def test_receive_oss_maintenance_notification(self):
             ],
         )
 
-        # execute a commands that will receive the notification
+        # execute a command that will receive the notification
         res = self.cluster.set("anyprefix:{3}:k", "VAL")
         assert res is True
 

tests/maint_notifications/test_maint_notifications.py

@@ -393,8 +393,6 @@ def test_init_with_defaults(self):
             assert notification.id == 1
             assert notification.ttl == OSSNodeMigratingNotification.DEFAULT_TTL
             assert notification.creation_time == 1000
-            assert notification.src_node is None
-            assert notification.dest_node is None
             assert notification.slots is None
 
     def test_init_with_all_parameters(self):
@@ -403,15 +401,11 @@ def test_init_with_all_parameters(self):
             slots = [1, 2, 3, 4, 5]
             notification = OSSNodeMigratingNotification(
                 id=1,
-                src_node="127.0.0.1:6379",
-                dest_node="127.0.0.1:6380",
                 slots=slots,
             )
             assert notification.id == 1
             assert notification.ttl == OSSNodeMigratingNotification.DEFAULT_TTL
             assert notification.creation_time == 1000
-            assert notification.src_node == "127.0.0.1:6379"
-            assert notification.dest_node == "127.0.0.1:6380"
             assert notification.slots == slots
 
     def test_default_ttl(self):
@@ -425,8 +419,6 @@ def test_repr(self):
         with patch("time.monotonic", return_value=1000):
             notification = OSSNodeMigratingNotification(
                 id=1,
-                src_node="127.0.0.1:6379",
-                dest_node="127.0.0.1:6380",
                 slots=[1, 2, 3],
             )
 
@@ -442,14 +434,10 @@ def test_equality_same_id_and_type(self):
         """Test equality for notifications with same id and type."""
         notification1 = OSSNodeMigratingNotification(
             id=1,
-            src_node="127.0.0.1:6379",
-            dest_node="127.0.0.1:6380",
             slots=[1, 2, 3],
         )
         notification2 = OSSNodeMigratingNotification(
             id=1,
-            src_node="127.0.0.1:6381",
-            dest_node="127.0.0.1:6382",
             slots=[4, 5, 6],
         )
         # Should be equal because id and type are the same
@@ -471,14 +459,10 @@ def test_hash_same_id_and_type(self):
         """Test hash for notifications with same id and type."""
         notification1 = OSSNodeMigratingNotification(
             id=1,
-            src_node="127.0.0.1:6379",
-            dest_node="127.0.0.1:6380",
             slots=[1, 2, 3],
         )
         notification2 = OSSNodeMigratingNotification(
             id=1,
-            src_node="127.0.0.1:6381",
-            dest_node="127.0.0.1:6382",
             slots=[4, 5, 6],
         )
         # Should have same hash because id and type are the same