Repository: redis/redis-py
Pull request #1765 (2021-12-02 08:59:13)

Improved RedisCluster's reinitialize_steps and documentation
### Pull Request check-list

_Please make sure to review and check all of these items:_

- [ ] Does `$ tox` pass with this change (including linting)?
- [ ] Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
- [ ] Is the new or changed code fully tested?
- [ ] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
- [ ] Is there an example added to the examples folder (if applicable)?

_NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open._

### Description of change

_Please provide a description of the change here._
1. RedisCluster's reinitialize_steps have been refactored to moved_reinitialize_steps and the documentation has been improved
2. Destructor added to ClusterNode to close connection pool when deleted
3. Added a check to make sure the startup nodes have cluster mode enabled

codecov-commenter: # [Codecov](https://codecov.io/gh/redis/redis-py/pull/1765?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) Report
> Merging [#1765](https://codecov.io/gh/redis/redis-py/pull/1765?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) (d99d970) into [master](https://codecov.io/gh/redis/redis-py/commit/d4252277a9dafed5af34b3f40ed7a57fc952d273?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) (d425227) will **decrease** coverage by `0.29%`.
> The diff coverage is `95.05%`.

[![Impacted file tree graph](https://codecov.io/gh/redis/redis-py/pull/1765/graphs/tree.svg?width=650&height=150&src=pr&token=yenl5fzxxr&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis)](https://codecov.io/gh/redis/redis-py/pull/1765?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis)

```diff
@@            Coverage Diff             @@
##           master    #1765      +/-   ##
==========================================
- Coverage   90.91%   90.61%   -0.30%     
==========================================
  Files          70       74       +4     
  Lines       14994    14967      -27     
==========================================
- Hits        13632    13563      -69     
- Misses       1362     1404      +42     
```


| [Impacted Files](https://codecov.io/gh/redis/redis-py/pull/1765?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) | Coverage Δ | |
|---|---|---|
| [setup.py](https://codecov.io/gh/redis/redis-py/pull/1765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-c2V0dXAucHk=) | `0.00% <ø> (ø)` | |
| [tests/test\_bloom.py](https://codecov.io/gh/redis/redis-py/pull/1765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9ibG9vbS5weQ==) | `90.54% <90.54%> (ø)` | |
| [redis/commands/bf/commands.py](https://codecov.io/gh/redis/redis-py/pull/1765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvY29tbWFuZHMvYmYvY29tbWFuZHMucHk=) | `95.04% <95.04%> (ø)` | |
| [redis/cluster.py](https://codecov.io/gh/redis/redis-py/pull/1765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvY2x1c3Rlci5weQ==) | `89.81% <100.00%> (+0.02%)` | :arrow_up: |
| [redis/commands/bf/\_\_init\_\_.py](https://codecov.io/gh/redis/redis-py/pull/1765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvY29tbWFuZHMvYmYvX19pbml0X18ucHk=) | `100.00% <100.00%> (ø)` | |
| [redis/commands/bf/info.py](https://codecov.io/gh/redis/redis-py/pull/1765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvY29tbWFuZHMvYmYvaW5mby5weQ==) | `100.00% <100.00%> (ø)` | |
| [redis/commands/redismodules.py](https://codecov.io/gh/redis/redis-py/pull/1765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvY29tbWFuZHMvcmVkaXNtb2R1bGVzLnB5) | `100.00% <100.00%> (ø)` | |
| [tests/test\_cluster.py](https://codecov.io/gh/redis/redis-py/pull/1765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9jbHVzdGVyLnB5) | `96.88% <100.00%> (-0.37%)` | :arrow_down: |
| [redis/retry.py](https://codecov.io/gh/redis/redis-py/pull/1765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvcmV0cnkucHk=) | `95.23% <0.00%> (-4.77%)` | :arrow_down: |
| [tests/test\_graph\_utils/test\_edge.py](https://codecov.io/gh/redis/redis-py/pull/1765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9ncmFwaF91dGlscy90ZXN0X2VkZ2UucHk=) | `93.93% <0.00%> (-3.36%)` | :arrow_down: |
| ... and [35 more](https://codecov.io/gh/redis/redis-py/pull/1765/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/redis/redis-py/pull/1765?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/redis/redis-py/pull/1765?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis). Last update [42101fc...d99d970](https://codecov.io/gh/redis/redis-py/pull/1765?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis).


Patch:
diff --git a/redis/cluster.py b/redis/cluster.py
index eead2b4d..c5634a05 100644
--- a/redis/cluster.py
+++ b/redis/cluster.py
@@ -414,14 +414,25 @@ class RedisCluster(RedisClusterCommands):
              stale data.
              When set to true, read commands will be assigned between the
              primary and its replications in a Round-Robin manner.
-         :cluster_error_retry_attempts: 'int'
+        :cluster_error_retry_attempts: 'int'
              Retry command execution attempts when encountering ClusterDownError
              or ConnectionError
-         :retry_on_timeout: 'bool'
+        :retry_on_timeout: 'bool'
              To specify a retry policy, first set `retry_on_timeout` to `True`
              then set `retry` to a valid `Retry` object
-         :retry: 'Retry'
+        :retry: 'Retry'
               a `Retry` object
+        :reinitialize_steps: 'int'
+            Specifies the number of MOVED errors that need to occur before
+            reinitializing the whole cluster topology. If a MOVED error occurs
+            and the cluster does not need to be reinitialized on this current
+            error handling, only the MOVED slot will be patched with the
+            redirected node.
+            To reinitialize the cluster on every MOVED error, set
+            reinitialize_steps to 1.
+            To avoid reinitializing the cluster on moved errors, set
+            reinitialize_steps to 0.
+
          :**kwargs:
              Extra arguments that will be sent into Redis instance when created
              (See Official redis-py doc for supported kwargs
@@ -727,7 +738,9 @@ class RedisCluster(RedisClusterCommands):
             return [node]
 
     def _should_reinitialized(self):
-        # In order not to reinitialize the cluster, the user can set
+        # To reinitialize the cluster on every MOVED error,
+        # set reinitialize_steps to 1.
+        # To avoid reinitializing the cluster on moved errors, set
         # reinitialize_steps to 0.
         if self.reinitialize_steps == 0:
             return False
@@ -958,8 +971,8 @@ class RedisCluster(RedisClusterCommands):
                 # redirected node output and try again. If MovedError exceeds
                 # 'reinitialize_steps' number of times, we will force
                 # reinitializing the tables, and then try again.
-                # 'reinitialize_steps' counter will increase faster when the
-                # same client object is shared between multiple threads. To
+                # 'reinitialize_steps' counter will increase faster when
+                # the same client object is shared between multiple threads. To
                 # reduce the frequency you can set this variable in the
                 # RedisCluster constructor.
                 log.exception("MovedError")
@@ -1055,6 +1068,10 @@ class ClusterNode:
     def __eq__(self, obj):
         return isinstance(obj, ClusterNode) and obj.name == self.name
 
+    def __del__(self):
+        if self.redis_connection is not None:
+            self.redis_connection.close()
+
 
 class LoadBalancer:
     """
@@ -1300,6 +1317,11 @@ class NodesManager:
                         startup_node.host, startup_node.port, **copy_kwargs
                     )
                     self.startup_nodes[startup_node.name].redis_connection = r
+                # Make sure cluster mode is enabled on this node
+                if bool(r.info().get("cluster_enabled")) is False:
+                    raise RedisClusterException(
+                        "Cluster mode is not enabled on this node"
+                    )
                 cluster_slots = r.execute_command("CLUSTER SLOTS")
                 startup_nodes_reachable = True
             except (ConnectionError, TimeoutError) as e:
@@ -1327,7 +1349,7 @@ class NodesManager:
                 message = e.__str__()
                 raise RedisClusterException(
                     'ERROR sending "cluster slots" command to redis '
-                    f"server: {startup_node}. error: {message}"
+                    f"server {startup_node.name}. error: {message}"
                 )
 
             # CLUSTER SLOTS command results in the following output:
diff --git a/redis/commands/core.py b/redis/commands/core.py
index 462fba7f..fec3095c 100644
--- a/redis/commands/core.py
+++ b/redis/commands/core.py
@@ -333,6 +333,16 @@ class ManagementCommands:
             pieces.append("SCHEDULE")
         return self.execute_command("BGSAVE", *pieces, **kwargs)
 
+    def role(self):
+        """
+        Provide information on the role of a Redis instance in
+        the context of replication, by returning if the instance
+        is currently a master, slave, or sentinel.
+
+        For more information check https://redis.io/commands/role
+        """
+        return self.execute_command("ROLE")
+
     def client_kill(self, address, **kwargs):
         """Disconnects the client at ``address`` (ip:port)
 
@@ -864,11 +874,15 @@ class ManagementCommands:
 
         For more information check https://redis.io/commands/slowlog-get
         """
+        from redis.client import NEVER_DECODE
+
         args = ["SLOWLOG GET"]
         if num is not None:
             args.append(num)
         decode_responses = self.get_connection_kwargs().get("decode_responses", False)
-        return self.execute_command(*args, decode_responses=decode_responses, **kwargs)
+        if decode_responses is True:
+            kwargs[NEVER_DECODE] = []
+        return self.execute_command(*args, **kwargs)
 
     def slowlog_len(self, **kwargs):
         """
diff --git a/tox.ini b/tox.ini
index f4eaedc8..32d1680a 100644
--- a/tox.ini
+++ b/tox.ini
@@ -73,7 +73,7 @@ healtcheck_cmd = python -c "import socket;print(True) if 0 == socket.socket(sock
 
 [docker:lots-of-pythons]
 name = lots-of-pythons
-image = redisfab/lots-of-pythons
+image = redisfab/lots-of-pythons:latest
 volumes =
     bind:rw:{toxinidir}:/data
 


Test patch:
diff --git a/tests/test_cluster.py b/tests/test_cluster.py
index b76ed809..4087d335 100644
--- a/tests/test_cluster.py
+++ b/tests/test_cluster.py
@@ -84,6 +84,7 @@ def get_mocked_redis_client(func=None, *args, **kwargs):
     """
     cluster_slots = kwargs.pop("cluster_slots", default_cluster_slots)
     coverage_res = kwargs.pop("coverage_result", "yes")
+    cluster_enabled = kwargs.pop("cluster_enabled", True)
     with patch.object(Redis, "execute_command") as execute_command_mock:
 
         def execute_command(*_args, **_kwargs):
@@ -92,7 +93,9 @@ def get_mocked_redis_client(func=None, *args, **kwargs):
                 return mock_cluster_slots
             elif _args[0] == "COMMAND":
                 return {"get": [], "set": []}
-            elif _args[1] == "cluster-require-full-coverage":
+            elif _args[0] == "INFO":
+                return {"cluster_enabled": cluster_enabled}
+            elif len(_args) > 1 and _args[1] == "cluster-require-full-coverage":
                 return {"cluster-require-full-coverage": coverage_res}
             elif func is not None:
                 return func(*args, **kwargs)
@@ -1974,6 +1977,17 @@ class TestNodesManager:
 
         assert len(n_manager.nodes_cache) == 6
 
+    def test_init_slots_cache_cluster_mode_disabled(self):
+        """
+        Test that creating a RedisCluster failes if one of the startup nodes
+        has cluster mode disabled
+        """
+        with pytest.raises(RedisClusterException) as e:
+            get_mocked_redis_client(
+                host=default_host, port=default_port, cluster_enabled=False
+            )
+            assert "Cluster mode is not enabled on this node" in str(e.value)
+
     def test_empty_startup_nodes(self):
         """
         It should not be possible to create a node manager with no nodes
@@ -2044,6 +2058,8 @@ class TestNodesManager:
                 def execute_command(*args, **kwargs):
                     if args[0] == "CLUSTER SLOTS":
                         return result
+                    elif args[0] == "INFO":
+                        return {"cluster_enabled": True}
                     elif args[1] == "cluster-require-full-coverage":
                         return {"cluster-require-full-coverage": "yes"}
                     else:
@@ -2108,6 +2124,8 @@ class TestNodesManager:
                                 ["127.0.0.1", 7002, "node_2"],
                             ],
                         ]
+                    elif args[0] == "INFO":
+                        return {"cluster_enabled": True}
                     elif args[1] == "cluster-require-full-coverage":
                         return {"cluster-require-full-coverage": "yes"}
 
diff --git a/tests/test_commands.py b/tests/test_commands.py
index 7c7d0f3d..556df840 100644
--- a/tests/test_commands.py
+++ b/tests/test_commands.py
@@ -653,6 +653,13 @@ class TestRedisCommands:
     def test_quit(self, r):
         assert r.quit()
 
+    @skip_if_server_version_lt("2.8.12")
+    @pytest.mark.onlynoncluster
+    def test_role(self, r):
+        assert r.role()[0] == b"master"
+        assert isinstance(r.role()[1], int)
+        assert isinstance(r.role()[2], list)
+
     @pytest.mark.onlynoncluster
     def test_slowlog_get(self, r, slowlog):
         assert r.slowlog_reset()
diff --git a/tests/test_connection_pool.py b/tests/test_connection_pool.py
index 276e77cd..138fcad4 100644
--- a/tests/test_connection_pool.py
+++ b/tests/test_connection_pool.py
@@ -450,7 +450,7 @@ class TestConnectionPoolUnixSocketURLParsing:
             pass
 
         pool = redis.ConnectionPool.from_url(
-            'unix:///socket', connection_class=MyConnection
+            "unix:///socket", connection_class=MyConnection
         )
         assert pool.connection_class == MyConnection
 
@@ -469,7 +469,7 @@ class TestSSLConnectionURLParsing:
             pass
 
         pool = redis.ConnectionPool.from_url(
-            'rediss://my.host', connection_class=MyConnection
+            "rediss://my.host", connection_class=MyConnection
         )
         assert pool.connection_class == MyConnection