Applying review comments.

Author: petyaslavova

docs/retry.rst

@@ -49,16 +49,11 @@ Retry in Redis Cluster
 Retry behaviour in Redis Cluster is a little bit different from Standalone:
 
 * ``retry``: :class:`~.Retry` instance with a :ref:`backoff-label` strategy and the max number of retries, default value is ``Retry(ExponentialWithJitterBackoff(base=1, cap=10), cluster_error_retry_attempts)``
-* ``cluster_error_retry_attempts``: number of times to retry before raising an error when :class:`~.TimeoutError` or :class:`~.ConnectionError` or :class:`~.ClusterDownError` or :class:`~.SlotNotCoveredError` are encountered, default value is ``3``
+* ``cluster_error_retry_attempts``: number of times to retry before raising an error when :class:`~.TimeoutError`, :class:`~.ConnectionError`, :class:`~.ClusterDownError` or :class:`~.SlotNotCoveredError` are encountered, default value is ``3``
     * This argument is deprecated - it is used to initialize the number of retries for the retry object,
       only in the case when the ``retry`` object is not provided.
       When the ``retry`` argument is provided, the ``cluster_error_retry_attempts`` argument is ignored!
 
-* Starting from version 6.0.0 of the library, the default retry policy for the nodes connections is without retries.
-  This means that if a connection to a node fails, the lower level connection will not retry the connection.
-  Instead, it will raise a :class:`~.ConnectionError` to the cluster level call., where it will be retried.
-  This is done to avoid blocking the cluster client for too long in case of a node failure.
-
 * The retry object is not yet fully utilized in the cluster client.
   The retry object is used only to determine the number of retries for the cluster level calls.
 

redis/asyncio/cluster.py

@@ -565,9 +565,6 @@ def get_connection_kwargs(self) -> Dict[str, Optional[Any]]:
         """Get the kwargs passed to :class:`~redis.asyncio.connection.Connection`."""
         return self.connection_kwargs
 
-    def get_retry(self) -> Retry:
-        return self.retry
-
     def set_retry(self, retry: Retry) -> None:
         if not isinstance(retry, Retry):
             raise TypeError(

redis/asyncio/retry.py

@@ -53,8 +53,6 @@ def update_retries(self, value: int) -> None:
         """
         Set the number of retries.
         """
-        if not isinstance(value, int):
-            raise ValueError("Retries must be an integer.")
         self._retries = value
 
     async def call_with_retry(

redis/cluster.py

@@ -610,7 +610,7 @@ def __init__(
             # and there we provide retry configuration without retries allowed.
             # The retries should be handled on cluster client level.
             raise RedisClusterException(
-                "Argument 'retry' is not possible to be used in kwargs when in cluster mode"
+                "The 'retry' argument cannot be used in kwargs when running in cluster mode."
             )
 
         # Get the startup node/s
@@ -800,9 +800,6 @@ def set_default_node(self, node):
         self.nodes_manager.default_node = node
         return True
 
-    def get_retry(self) -> Retry:
-        return self.retry
-
     def set_retry(self, retry: Retry) -> None:
         if not isinstance(retry, Retry):
             raise TypeError("retry must be a valid instance of redis.retry.Retry")

redis/retry.py

@@ -54,8 +54,6 @@ def update_retries(self, value: int) -> None:
         """
         Set the number of retries.
         """
-        if not isinstance(value, int):
-            raise ValueError("Retries count must be an integer.")
         self._retries = value
 
     def call_with_retry(

tests/test_asyncio/test_cluster.py

@@ -371,8 +371,8 @@ async def test_cluster_set_get_retry_object(self, request: FixtureRequest):
         retry = Retry(NoBackoff(), 2)
         url = request.config.getoption("--redis-url")
         async with RedisCluster.from_url(url, retry=retry) as r:
-            assert r.get_retry().get_retries() == retry.get_retries()
-            assert isinstance(r.get_retry()._backoff, NoBackoff)
+            assert r.retry.get_retries() == retry.get_retries()
+            assert isinstance(r.retry._backoff, NoBackoff)
             for node in r.get_nodes():
                 # validate nodes lower level connections default
                 # retry policy is applied
@@ -385,8 +385,8 @@ async def test_cluster_set_get_retry_object(self, request: FixtureRequest):
             # Change retry policy
             new_retry = Retry(ExponentialBackoff(), 3)
             r.set_retry(new_retry)
-            assert r.get_retry().get_retries() == new_retry.get_retries()
-            assert isinstance(r.get_retry()._backoff, ExponentialBackoff)
+            assert r.retry.get_retries() == new_retry.get_retries()
+            assert isinstance(r.retry._backoff, ExponentialBackoff)
             for node in r.get_nodes():
                 # validate nodes lower level connections are not affected
                 n_retry = node.acquire_connection().retry
@@ -423,7 +423,7 @@ async def test_cluster_retry_object(self, request: FixtureRequest) -> None:
         retry = Retry(ExponentialBackoff(10, 5), 5)
         async with RedisCluster.from_url(url, retry=retry) as rc_custom_retry:
             # Test custom retry
-            assert rc_custom_retry.get_retry() == retry
+            assert rc_custom_retry.retry == retry
             # validate nodes connections are using the default retry for
             # lower level connections when client is created through 'from_url' method
             # with specified retry object
@@ -438,12 +438,12 @@ async def test_cluster_retry_object(self, request: FixtureRequest) -> None:
             url, cluster_error_retry_attempts=0
         ) as rc_no_retries:
             # Test no cluster retries
-            assert rc_no_retries.get_retry().get_retries() == 0
+            assert rc_no_retries.retry.get_retries() == 0
 
         async with RedisCluster.from_url(
             url, retry=Retry(NoBackoff(), 0)
         ) as rc_no_retries:
-            assert rc_no_retries.get_retry().get_retries() == 0
+            assert rc_no_retries.retry.get_retries() == 0
 
     async def test_empty_startup_nodes(self) -> None:
         """

tests/test_cluster.py

@@ -888,8 +888,8 @@ def moved_redirect_effect(connection, *args, **options):
     def test_cluster_get_set_retry_object(self, request):
         retry = Retry(NoBackoff(), 2)
         r = _get_client(RedisCluster, request, retry=retry)
-        assert r.get_retry().get_retries() == retry.get_retries()
-        assert isinstance(r.get_retry()._backoff, NoBackoff)
+        assert r.retry.get_retries() == retry.get_retries()
+        assert isinstance(r.retry._backoff, NoBackoff)
         for node in r.get_nodes():
             assert node.redis_connection.get_retry().get_retries() == 0
             assert isinstance(node.redis_connection.get_retry()._backoff, NoBackoff)
@@ -898,8 +898,8 @@ def test_cluster_get_set_retry_object(self, request):
         # Change retry policy
         new_retry = Retry(ExponentialBackoff(), 3)
         r.set_retry(new_retry)
-        assert r.get_retry().get_retries() == new_retry.get_retries()
-        assert isinstance(r.get_retry()._backoff, ExponentialBackoff)
+        assert r.retry.get_retries() == new_retry.get_retries()
+        assert isinstance(r.retry._backoff, ExponentialBackoff)
         for node in r.get_nodes():
             assert node.redis_connection.get_retry()._retries == 0
             assert isinstance(node.redis_connection.get_retry()._backoff, NoBackoff)
@@ -913,14 +913,14 @@ def test_cluster_retry_object(self, r) -> None:
         host = r.get_default_node().host
 
         # test default retry config
-        retry = r.get_retry()
+        retry = r.retry
         assert isinstance(retry, Retry)
         assert retry.get_retries() == 3
         assert isinstance(retry._backoff, type(ExponentialWithJitterBackoff()))
-        node1 = r.get_node(host, 16379).redis_connection
-        node2 = r.get_node(host, 16380).redis_connection
-        assert node1.get_retry()._retries == 0
-        assert node2.get_retry()._retries == 0
+        node1_connection = r.get_node(host, 16379).redis_connection
+        node2_connection = r.get_node(host, 16380).redis_connection
+        assert node1_connection.get_retry()._retries == 0
+        assert node2_connection.get_retry()._retries == 0
 
         # Test custom retry is not applied to nodes
         retry = Retry(ExponentialBackoff(10, 5), 5)

tests/test_retry.py

@@ -203,6 +203,22 @@ def test_client_retry_on_timeout(self, request):
                 finally:
                     assert parse_response.call_count == retries + 1
 
+    @pytest.mark.onlycluster
+    def test_get_set_retry_object_for_cluster_client(self, request):
+        retry = Retry(NoBackoff(), 2)
+        r = _get_client(Redis, request, retry_on_timeout=True, retry=retry)
+        exist_conn = r.connection_pool.get_connection()
+        assert r.retry._retries == retry._retries
+        assert isinstance(r.retry._backoff, NoBackoff)
+        new_retry_policy = Retry(ExponentialBackoff(), 3)
+        r.set_retry(new_retry_policy)
+        assert r.retry._retries == new_retry_policy._retries
+        assert isinstance(r.retry._backoff, ExponentialBackoff)
+        assert exist_conn.retry._retries == new_retry_policy._retries
+        new_conn = r.connection_pool.get_connection()
+        assert new_conn.retry._retries == new_retry_policy._retries
+
+    @pytest.mark.onlynoncluster
     def test_get_set_retry_object(self, request):
         retry = Retry(NoBackoff(), 2)
         r = _get_client(Redis, request, retry_on_timeout=True, retry=retry)