Repository: redis/redis-py
File: tests/test_scenario/test_hitless_upgrade.py (Python)

Code before:

        # wait for all command execution threads to stop
        for thread in commands_threads:
            thread.join()

        # validate that all connections are either disconnected
        # or connected to the new address
        connections = self._get_all_connections_in_pool(client)
        for conn in connections:
            if conn._sock is not None:
                assert conn.get_resolved_ip() == conn.host
                assert conn.maintenance_state == MaintenanceState.MOVING
                assert conn._sock.gettimeout() == RELAX_TIMEOUT
                assert conn.host != conn.orig_host_address
                assert not conn.should_reconnect()
            else:
                assert conn.maintenance_state == MaintenanceState.MOVING
                assert conn.socket_timeout == RELAX_TIMEOUT
                assert conn.host != conn.orig_host_address
                assert not conn.should_reconnect()

        # validate no errors were raised in the command execution threads
        assert errors.empty(), f"Errors occurred in threads: {errors.queue}"

        logging.info("Waiting for moving ttl to expire")
        time.sleep(15)
        bind_thread.join()

    @pytest.mark.timeout(300)  # 5 minutes timeout
    def test_new_connections_receive_moving(
        self,
        client_maint_events: Redis,
        fault_injector_client: FaultInjectorClient,
        endpoints_config: Dict[str, Any],
    ):
        logging.info("Creating one connection in the pool.")
        first_conn = client_maint_events.connection_pool.get_connection()

        logging.info("Executing rladmin migrate command...")
        migrate_thread = Thread(
            target=self._execute_migration,
            name="migrate_thread",
            args=(
                fault_injector_client,
                endpoints_config,
                self.target_node,
                self.empty_node,
            ),
        )
        migrate_thread.start()


Review comment (suggestion):
[nitpick] The hardcoded sleep value of 15 seconds should be replaced with a named constant or configuration variable to improve maintainability and make the timeout purpose clearer.

Code after:
            # Wait for all workers to finish and propagate any exceptions
            for f in futures:
                f.result()

        # validate that all connections are either disconnected
        # or connected to the new address
        connections = self._get_all_connections_in_pool(client)
        for conn in connections:
            if conn._sock is not None:
                assert conn.get_resolved_ip() == conn.host
                assert conn.maintenance_state == MaintenanceState.MOVING
                assert conn._sock.gettimeout() == RELAX_TIMEOUT
                assert conn.host != conn.orig_host_address
                assert not conn.should_reconnect()
            else:
                assert conn.maintenance_state == MaintenanceState.MOVING
                assert conn.socket_timeout == RELAX_TIMEOUT
                assert conn.host != conn.orig_host_address
                assert not conn.should_reconnect()

        # validate no errors were raised in the command execution threads
        assert errors.empty(), f"Errors occurred in threads: {errors.queue}"

        logging.info("Waiting for moving ttl to expire")
        time.sleep(DEFAULT_BIND_TTL)
        bind_thread.join()

    @pytest.mark.timeout(300)  # 5 minutes timeout
    def test_new_connections_receive_moving(
        self,
        client_maint_events: Redis,
        fault_injector_client: FaultInjectorClient,
        endpoints_config: Dict[str, Any],
    ):
        logging.info("Creating one connection in the pool.")
        first_conn = client_maint_events.connection_pool.get_connection()

        logging.info("Executing rladmin migrate command...")
        migrate_thread = Thread(
            target=self._execute_migration,
            name="migrate_thread",
            args=(
                fault_injector_client,
                endpoints_config,
                self.target_node,
                self.empty_node,
            ),
        )
        migrate_thread.start()

        logging.info("Waiting for MIGRATING push notifications...")