From 83a5eae77571e04e2fc30bbeb825f61d8ebcabf1 Mon Sep 17 00:00:00 2001 From: YuLun Shih Date: Mon, 22 Jun 2015 17:16:43 +0800 Subject: [PATCH 01/50] Fix typo --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 6019fa08..7c5d4001 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ This project is a port of `redis-rb-cluster` by antirez, with alot of added func The project is not dead but not much new development is done right now. I do awnser issue reports and pull requests as soon as possible and if you have a problem you can ping me inside the gitter channel that you can find here [![Gitter](https://badges.gitter.im/Join Chat.svg)](https://gitter.im/Grokzen/redis-py-cluster?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge) and i will help you out with problems or usage of this lib. -As of release `0.3.0` this project will be considered stable and usable in production. Just remember that if you are going to use redis cluster to please reda up on the documentation that you can find in the bottom of this Readme. It will contain usage examples and descriptions of what is implemented and what is not implemented and why things are the way they are. +As of release `0.3.0` this project will be considered stable and usable in production. Just remember that if you are going to use redis cluster to please read up on the documentation that you can find in the bottom of this Readme. It will contain usage examples and descriptions of what is implemented and what is not implemented and why things are the way they are. On the topic about porting/moving this code into `redis-py` there is currently work over here https://github.com/andymccurdy/redis-py/pull/604 that will bring cluster uspport based on this code. But my suggestion is that until that work is completed that you should use this lib. From cd14657cec137fa59a94a38a727c8360c0629a23 Mon Sep 17 00:00:00 2001 From: Yoshinari Takaoka Date: Sun, 28 Jun 2015 23:56:21 +0900 Subject: [PATCH 02/50] - newly implemented READONLY mode, scales reads using slave nodes. --- rediscluster/client.py | 10 ++-- rediscluster/connection.py | 35 +++++++++++++- rediscluster/nodemanager.py | 16 +++---- rediscluster/pipeline.py | 4 +- tests/conftest.py | 10 ++++ tests/test_cluster_connection_pool.py | 40 +++++++++++++++- tests/test_cluster_obj.py | 68 +++++++++++++++++++++++---- tests/test_node_manager.py | 31 ++++++++---- 8 files changed, 179 insertions(+), 35 deletions(-) diff --git a/rediscluster/client.py b/rediscluster/client.py index f5a80eb2..6a6e4da0 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -7,7 +7,7 @@ import time # rediscluster imports -from .connection import ClusterConnectionPool +from .connection import ClusterConnectionPool, ClusterReadOnlyConnectionPool from .exceptions import RedisClusterException, ClusterDownException from .pubsub import ClusterPubSub from .utils import ( @@ -82,13 +82,14 @@ class StrictRedisCluster(StrictRedis): ) def __init__(self, host=None, port=None, startup_nodes=None, max_connections=32, init_slot_cache=True, - pipeline_use_threads=True, **kwargs): + pipeline_use_threads=True, readonly_mode=False, **kwargs): """ startup_nodes --> List of nodes that initial bootstrapping can be done from host --> Can be used to point to a startup node port --> Can be used to point to a startup node max_connections --> Maximum number of connections that should be kept open at one time pipeline_use_threads -> By default, use threads in pipeline if this flag is set to True + readonly_mode --> enable READONLY mode. You can read possibly stale data from slave. **kwargs --> Extra arguments that will be sent into StrictRedis instance when created (See Official redis-py doc for supported kwargs [https://github.com/andymccurdy/redis-py/blob/master/redis/client.py]) @@ -107,7 +108,8 @@ def __init__(self, host=None, port=None, startup_nodes=None, max_connections=32, if host: startup_nodes.append({"host": host, "port": port if port else 7000}) - self.connection_pool = ClusterConnectionPool( + connection_pool_cls = ClusterReadOnlyConnectionPool if readonly_mode else ClusterConnectionPool + self.connection_pool = connection_pool_cls( startup_nodes=startup_nodes, init_slot_cache=init_slot_cache, max_connections=max_connections, @@ -157,7 +159,7 @@ def handle_cluster_command_exception(self, e): if info['action'] == "MOVED": self.refresh_table_asap = True node = self.connection_pool.nodes.set_node(info['host'], info['port'], server_type='master') - self.connection_pool.nodes.slots[info['slot']] = node + self.connection_pool.nodes.slots[info['slot']][0] = node elif info['action'] == "ASK": node = self.connection_pool.nodes.set_node(info['host'], info['port'], server_type='master') return {'name': node['name'], 'method': 'ask'} diff --git a/rediscluster/connection.py b/rediscluster/connection.py index fed30134..a343f4f9 100644 --- a/rediscluster/connection.py +++ b/rediscluster/connection.py @@ -2,6 +2,7 @@ # python std lib import os +import random import threading from contextlib import contextmanager from itertools import chain @@ -11,7 +12,9 @@ from .exceptions import RedisClusterException # 3rd party imports +from redis._compat import nativestr from redis.connection import ConnectionPool, Connection +from redis.exceptions import ConnectionError class ClusterConnection(Connection): @@ -19,6 +22,19 @@ class ClusterConnection(Connection): description_format = "ClusterConnection" +class ClusterReadOnlyConnection(Connection): + "Manages READONLY TCP communication to and from a Redis server" + description_format = "ClusterReadOnlyConnection" + + def on_connect(self): + "Initialize the connection, authenticate and select a database and send READONLY command" + super(ClusterReadOnlyConnection, self).on_connect() + + self.send_command('READONLY') + if nativestr(self.read_response()) != 'OK': + raise ConnectionError('READONLY command failed') + + class UnixDomainSocketConnection(Connection): description_format = "ClusterUnixDomainSocketConnection" @@ -198,7 +214,24 @@ def get_connection_by_node(self, node): return connection def get_node_by_slot(self, slot): - return self.nodes.slots[slot] + return self.nodes.slots[slot][0] + + +class ClusterReadOnlyConnectionPool(ClusterConnectionPool): + """ + Readonly connection pool for rediscluster + """ + + def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=ClusterReadOnlyConnection, max_connections=None, **connection_kwargs): + super(ClusterReadOnlyConnectionPool, self).__init__( + startup_nodes=startup_nodes, + init_slot_cache=init_slot_cache, + connection_class=connection_class, + max_connections=max_connections, + **connection_kwargs) + + def get_node_by_slot(self, slot): + return random.choice(self.nodes.slots[slot]) @contextmanager diff --git a/rediscluster/nodemanager.py b/rediscluster/nodemanager.py index 8a48aec7..b2deb10b 100644 --- a/rediscluster/nodemanager.py +++ b/rediscluster/nodemanager.py @@ -102,8 +102,6 @@ def initialize(self): for slot in cluster_slots: master_node = slot[2] - # Only store the master node as address for each slot. - # TODO: Slave nodes have to be fixed/patched in later... if master_node[0] == '': master_node[0] = node['host'] master_node[1] = int(master_node[1]) @@ -112,18 +110,18 @@ def initialize(self): for i in range(int(slot[0]), int(slot[1]) + 1): if i not in self.slots: - self.slots[i] = node + self.slots[i] = [node] + slave_nodes = [slot[j] for j in range(3, len(slot))] + for slave_node in slave_nodes: + target_slave_node = self.set_node(slave_node[0], slave_node[1], server_type='slave') + self.slots[i].append(target_slave_node) else: # Validate that 2 nodes want to use the same slot cache setup - if self.slots[i]['name'] != node['name']: - disagreements.append("{} vs {} on slot: {}".format(self.slots[i]['name'], node['name'], i)) + if self.slots[i][0]['name'] != node['name']: + disagreements.append("{} vs {} on slot: {}".format(self.slots[i][0]['name'], node['name'], i)) if len(disagreements) > 5: raise RedisClusterException("startup_nodes could not agree on a valid slots cache. %s" % ", ".join(disagreements)) - slave_nodes = [slot[i] for i in range(3, len(slot))] - for slave_node in slave_nodes: - self.set_node(slave_node[0], slave_node[1], server_type='slave') - self.populate_startup_nodes() self.refresh_table_asap = False diff --git a/rediscluster/pipeline.py b/rediscluster/pipeline.py index 08ccedd3..67e0433b 100644 --- a/rediscluster/pipeline.py +++ b/rediscluster/pipeline.py @@ -140,7 +140,7 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T if slot in ask_slots: node = ask_slots[slot] else: - node = self.connection_pool.nodes.slots[slot] + node = self.connection_pool.nodes.slots[slot][0] self.connection_pool.nodes.set_node_name(node) node_name = node['name'] @@ -211,7 +211,7 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T if redir['action'] == "MOVED": self.refresh_table_asap = True node = self.connection_pool.nodes.set_node(redir['host'], redir['port'], server_type='master') - self.connection_pool.nodes.slots[redir['slot']] = node + self.connection_pool.nodes.slots[redir['slot']][0] = node attempt.append(i) self._fail_on_redirect(allow_redirections) elif redir['action'] == "ASK": diff --git a/tests/conftest.py b/tests/conftest.py index 6920fbbf..50dbd9fc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -90,6 +90,16 @@ def r(request, **kwargs): return _init_client(request, cls=StrictRedisCluster, **kwargs) +@pytest.fixture() +def ro(request, **kwargs): + """ + Create a StrictRedisCluster instance with readonly mode + """ + params = {'readonly_mode': True} + params.update(kwargs) + return _init_client(request, cls=StrictRedisCluster, **params) + + @pytest.fixture() def s(request, **kwargs): """ diff --git a/tests/test_cluster_connection_pool.py b/tests/test_cluster_connection_pool.py index 69321a94..79b2073a 100644 --- a/tests/test_cluster_connection_pool.py +++ b/tests/test_cluster_connection_pool.py @@ -8,7 +8,9 @@ from threading import Thread # rediscluster imports -from rediscluster.connection import ClusterConnectionPool, ClusterConnection, UnixDomainSocketConnection +from rediscluster.connection import ( + ClusterConnectionPool, ClusterReadOnlyConnectionPool, + ClusterConnection, UnixDomainSocketConnection) from rediscluster.exceptions import RedisClusterException from tests.conftest import skip_if_server_version_lt @@ -122,6 +124,42 @@ def test_get_connection_blocked(self): assert unicode(ex.value).startswith("Only 'pubsub' commands can be used by get_connection()") +class TestReadOnlyConnectionPool(object): + def get_pool(self, connection_kwargs=None, max_connections=None): + connection_kwargs = connection_kwargs or {} + pool = ClusterReadOnlyConnectionPool( + max_connections=max_connections, + startup_nodes=[{"host": "127.0.0.1", "port": 7000}], + **connection_kwargs) + return pool + + def test_repr_contains_db_info_readonly(self): + connection_kwargs = {'host': 'localhost', 'port': 7000} + pool = self.get_pool(connection_kwargs=connection_kwargs) + expected = 'ClusterReadOnlyConnectionPool>' + assert repr(pool) == expected + + def test_max_connections(self): + pool = self.get_pool(max_connections=2) + pool.get_connection_by_node({"host": "127.0.0.1", "port": 7000}) + pool.get_connection_by_node({"host": "127.0.0.1", "port": 7001}) + with pytest.raises(RedisClusterException): + pool.get_connection_by_node({"host": "127.0.0.1", "port": 7000}) + + def test_get_node_by_slot(self): + """ + We can randomly get all nodes in readonly mode. + """ + pool = self.get_pool(connection_kwargs={}) + + expected_ports = {7000, 7003} + actual_ports = set() + for _ in range(0, 100): + node = pool.get_node_by_slot(0) + actual_ports.add(node['port']) + assert actual_ports == expected_ports + + class TestBlockingConnectionPool(object): def get_pool(self, connection_kwargs=None, max_connections=10, timeout=20): connection_kwargs = connection_kwargs or {} diff --git a/tests/test_cluster_obj.py b/tests/test_cluster_obj.py index 0dd9fa29..7855a1b9 100644 --- a/tests/test_cluster_obj.py +++ b/tests/test_cluster_obj.py @@ -6,7 +6,7 @@ # rediscluster imports from rediscluster import StrictRedisCluster -from rediscluster.connection import ClusterConnectionPool +from rediscluster.connection import ClusterConnectionPool, ClusterReadOnlyConnectionPool from rediscluster.exceptions import RedisClusterException from rediscluster.nodemanager import NodeManager from tests.conftest import _get_client, skip_if_server_version_lt @@ -14,7 +14,7 @@ # 3rd party imports from mock import patch, Mock from redis.exceptions import ResponseError -from redis._compat import unicode +from redis._compat import b, unicode import pytest @@ -58,6 +58,13 @@ def test_empty_startup_nodes(s): assert unicode(ex.value).startswith("No startup nodes provided"), unicode(ex.value) +def test_readonly_instance(ro): + """ + Test that readonly_mode=True instance has ClusterReadOnlyConnectionPool + """ + assert isinstance(ro.connection_pool, ClusterReadOnlyConnectionPool) + + def test_blocked_commands(r): """ These commands should be blocked and raise RedisClusterException @@ -115,12 +122,12 @@ def side_effect_rebuild_slots_cache(self): self.slots = {} for i in range(0, 16383): - self.slots[i] = { + self.slots[i] = [{ 'host': '127.0.0.1', 'server_type': 'master', 'port': 7006, 'name': '127.0.0.1:7006', - } + }] # Second call should map all to 7007 def map_7007(self): @@ -128,12 +135,12 @@ def map_7007(self): self.slots = {} for i in range(0, 16383): - self.slots[i] = { + self.slots[i] = [{ 'host': '127.0.0.1', 'server_type': 'master', 'port': 7007, 'name': '127.0.0.1:7007', - } + }] # First call should map all to 7006 init_mock.side_effect = map_7007 @@ -165,7 +172,7 @@ def test_moved_exception_handling(r): resp.message = "MOVED 1337 127.0.0.1:7000" r.handle_cluster_command_exception(resp) assert r.refresh_table_asap is True - assert r.connection_pool.nodes.slots[1337] == { + assert r.connection_pool.nodes.slots[1337][0] == { "host": "127.0.0.1", "port": 7000, "name": "127.0.0.1:7000", @@ -240,12 +247,12 @@ def test_refresh_table_asap(): mock_initialize.return_value = None r = StrictRedisCluster(host="127.0.0.1", port=7000) - r.connection_pool.nodes.slots[12182] = { + r.connection_pool.nodes.slots[12182] = [{ "host": "127.0.0.1", "port": 7002, "name": "127.0.0.1:7002", "server_type": "master", - } + }] r.refresh_table_asap = True i = len(mock_initialize.mock_calls) @@ -375,3 +382,46 @@ def ok_response(connection, command_name, **options): p.parse_response = m p.set("foo", "bar") assert p.execute() == ["MOCK_OK"] + + +def test_moved_redirection_on_slave_with_default_client(): + """ + Test that the client returns 'Too many Cluster redirections error' + with default (readonly_mode=False) client when we connect always to slave. + """ + + with patch.object(ClusterConnectionPool, 'get_node_by_slot') as return_slave_mock: + return_slave_mock.return_value = { + 'name': '127.0.0.1:7004', + 'host': '127.0.0.1', + 'port': 7004, + 'server_type': 'slave', + } + + normal_client = StrictRedisCluster(host="127.0.0.1", port=7000) + with pytest.raises(RedisClusterException) as ex: + normal_client.get('foo') + assert unicode(ex.value).startswith('Too many Cluster redirections') + + +def test_moved_redirection_on_slave_with_readonly_mode_client(sr): + """ + Test that the client can get value normally with readonly mode + when we connect always to slave. + """ + + # we assume this key is set on 127.0.0.1:7001 + sr.set('foo16706', 'foo') + import time + time.sleep(1) + + with patch.object(ClusterConnectionPool, 'get_node_by_slot') as return_slave_mock: + return_slave_mock.return_value = { + 'name': '127.0.0.1:7004', + 'host': '127.0.0.1', + 'port': 7004, + 'server_type': 'slave', + } + + readonly_client = StrictRedisCluster(host="127.0.0.1", port=7000, readonly_mode=True) + assert b('foo') == readonly_client.get('foo16706') diff --git a/tests/test_node_manager.py b/tests/test_node_manager.py index 87b920d5..21add47c 100644 --- a/tests/test_node_manager.py +++ b/tests/test_node_manager.py @@ -74,14 +74,26 @@ def test_init_slots_cache(s): Test that slots cache can in initialized and all slots are covered """ good_slots_resp = [ - [0, 5460, [b'127.0.0.1', 7000], [b'127.0.0.1', 7003]], - [5461, 10922, [b'127.0.0.1', 7001], [b'127.0.0.1', 7004]], - [10923, 16383, [b'127.0.0.1', 7002], [b'127.0.0.1', 7005]], + [0, 5460, [b'127.0.0.1', 7000], [b'127.0.0.2', 7003]], + [5461, 10922, [b'127.0.0.1', 7001], [b'127.0.0.2', 7004]], + [10923, 16383, [b'127.0.0.1', 7002], [b'127.0.0.2', 7005]], ] s.execute_command = lambda *args: good_slots_resp s.connection_pool.nodes.initialize() assert len(s.connection_pool.nodes.slots) == NodeManager.RedisClusterHashSlots + for slot_info in good_slots_resp: + all_hosts = ['127.0.0.1', '127.0.0.2'] + all_ports = [7000, 7001, 7002, 7003, 7004, 7005] + slot_start = slot_info[0] + slot_end = slot_info[1] + for i in range(slot_start, slot_end + 1): + assert len(s.connection_pool.nodes.slots[i]) == len(slot_info[2:]) + assert s.connection_pool.nodes.slots[i][0]['host'] in all_hosts + assert s.connection_pool.nodes.slots[i][1]['host'] in all_hosts + assert s.connection_pool.nodes.slots[i][0]['port'] in all_ports + assert s.connection_pool.nodes.slots[i][1]['port'] in all_ports + assert len(s.connection_pool.nodes.nodes) == 6 @@ -300,12 +312,13 @@ def test_cluster_one_instance(): }} assert len(n.slots) == 16384 - assert n.slots[0] == { - "host": "127.0.0.1", - "name": "127.0.0.1:7006", - "port": 7006, - "server_type": "master", - } + for i in range(0, 16384): + assert n.slots[i] == [{ + "host": "127.0.0.1", + "name": "127.0.0.1:7006", + "port": 7006, + "server_type": "master", + }] def test_init_with_down_node(): From bcff3c3c4d87fabb73aefe116fb4c791dda1022f Mon Sep 17 00:00:00 2001 From: Yoshinari Takaoka Date: Mon, 29 Jun 2015 01:21:40 +0900 Subject: [PATCH 03/50] - added READONLY mode documentation. --- README.md | 1 + docs/Readonly_mode.md | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 docs/Readonly_mode.md diff --git a/README.md b/README.md index 7c5d4001..0b2b747d 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,7 @@ More detailed documentation can be found in `docs` folder. - [Pipelines](docs/Pipelines.md) - [Threaded Pipeline support](docs/Threads.md) - [Cluster Management class](docs/ClusterMgt.md) +- [READONLY mode](docs/Readonly_mode.md) - [Authors](docs/Authors) diff --git a/docs/Readonly_mode.md b/docs/Readonly_mode.md new file mode 100644 index 00000000..3d227532 --- /dev/null +++ b/docs/Readonly_mode.md @@ -0,0 +1,30 @@ +# READONLY mode + +By default, Redis Cluster always returns MOVE redirection response on accessing slave node. You can overcome this limitation [for scaling read with READONLY mode](http://redis.io/topics/cluster-spec#scaling-reads-using-slave-nodes). + +redis-py-cluster also implements this mode. You can access slave by passing `readonly_mode=True` to StrictRedisCluster (or RedisCluster) constructor. + +```python +>>> from rediscluster import StrictRedisCluster +>>> startup_nodes = [{"host": "127.0.0.1", "port": "7000"}] +>>> rc = StrictRedisCluster(startup_nodes=startup_nodes, decode_responses=True) +>>> rc.set("foo16706", "bar") +True +>>> rc_readonly = StrictRedisCluster(startup_nodes=startup_nodes, decode_responses=True, readonly_mode=True) +>>> rc_readonly.get("foo16706") +u'bar' +``` + +But this mode has some downside or limitations. + +- It is possible that you cannot get the latest data from READONLY mode enabled object because Redis implements asynchronous replication. +- You MUST NOT use SET related operation with READONLY mode enabled object, otherwise you can possibly get 'Too many Cluster redirections' error because we choose master and its slave nodes randomly. + - You should use get related stuff only. + +```python +>>> rc_readonly = StrictRedisCluster(startup_nodes=startup_nodes, decode_responses=True, readonly_mode=True) +>>> # NO: This works in almost case, but possibly emits Too many Cluster redirections error... +>>> rc_readonly.set('foo', 'bar') +>>> # OK: You should always use get related stuff... +>>> rc_readonly.get('foo') +``` From 9b8ea8cd76c1c61730146ba3bd3dcd1ea71d7212 Mon Sep 17 00:00:00 2001 From: Yoshinari Takaoka Date: Thu, 2 Jul 2015 00:29:47 +0900 Subject: [PATCH 04/50] - added pipeline support on READONLY mode. --- docs/Readonly_mode.md | 17 ++++- rediscluster/client.py | 5 +- rediscluster/connection.py | 5 +- rediscluster/pipeline.py | 5 +- tests/test_cluster_connection_pool.py | 7 +++ tests/test_cluster_obj.py | 67 +++++++++++++++----- tests/test_pipeline.py | 89 +++++++++++++++++++++++++++ 7 files changed, 174 insertions(+), 21 deletions(-) diff --git a/docs/Readonly_mode.md b/docs/Readonly_mode.md index 3d227532..7bd03bfe 100644 --- a/docs/Readonly_mode.md +++ b/docs/Readonly_mode.md @@ -9,17 +9,32 @@ redis-py-cluster also implements this mode. You can access slave by passing `rea >>> startup_nodes = [{"host": "127.0.0.1", "port": "7000"}] >>> rc = StrictRedisCluster(startup_nodes=startup_nodes, decode_responses=True) >>> rc.set("foo16706", "bar") +>>> rc.set("foo81", "foo") True >>> rc_readonly = StrictRedisCluster(startup_nodes=startup_nodes, decode_responses=True, readonly_mode=True) >>> rc_readonly.get("foo16706") u'bar' +>>> rc_readonly.get("foo81") +u'foo' +``` + +We can use pipeline via `readonly_mode=True` object. + +```python +>>> with rc_readonly.pipeline() as readonly_pipe: +... readonly_pipe.get('foo81') +... readonly_pipe.get('foo16706') +... readonly_pipe.execute() +... +[u'foo', u'bar'] ``` But this mode has some downside or limitations. - It is possible that you cannot get the latest data from READONLY mode enabled object because Redis implements asynchronous replication. -- You MUST NOT use SET related operation with READONLY mode enabled object, otherwise you can possibly get 'Too many Cluster redirections' error because we choose master and its slave nodes randomly. +- **You MUST NOT use SET related operation with READONLY mode enabled object**, otherwise you can possibly get 'Too many Cluster redirections' error because we choose master and its slave nodes randomly. - You should use get related stuff only. + - Ditto with pipeline, otherwise you can get 'Command # X (XXXX) of pipeline: MOVED' error. ```python >>> rc_readonly = StrictRedisCluster(startup_nodes=startup_nodes, decode_responses=True, readonly_mode=True) diff --git a/rediscluster/client.py b/rediscluster/client.py index 6a6e4da0..24009f43 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -287,7 +287,10 @@ def execute_command(self, *args, **kwargs): r = self.connection_pool.get_random_connection() try_random_node = False else: - node = self.connection_pool.get_node_by_slot(slot) + if self.refresh_table_asap: # MOVED + node = self.connection_pool.get_master_node_by_slot(slot) + else: + node = self.connection_pool.get_node_by_slot(slot) r = self.connection_pool.get_connection_by_node(node) try: diff --git a/rediscluster/connection.py b/rediscluster/connection.py index a343f4f9..f0f48e23 100644 --- a/rediscluster/connection.py +++ b/rediscluster/connection.py @@ -213,9 +213,12 @@ def get_connection_by_node(self, node): return connection - def get_node_by_slot(self, slot): + def get_master_node_by_slot(self, slot): return self.nodes.slots[slot][0] + def get_node_by_slot(self, slot): + return self.get_master_node_by_slot(slot) + class ClusterReadOnlyConnectionPool(ClusterConnectionPool): """ diff --git a/rediscluster/pipeline.py b/rediscluster/pipeline.py index 67e0433b..73b8a7a8 100644 --- a/rediscluster/pipeline.py +++ b/rediscluster/pipeline.py @@ -140,7 +140,10 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T if slot in ask_slots: node = ask_slots[slot] else: - node = self.connection_pool.nodes.slots[slot][0] + if self.refresh_table_asap: # MOVED + node = self.connection_pool.get_master_node_by_slot(slot) + else: + node = self.connection_pool.get_node_by_slot(slot) self.connection_pool.nodes.set_node_name(node) node_name = node['name'] diff --git a/tests/test_cluster_connection_pool.py b/tests/test_cluster_connection_pool.py index 79b2073a..de9fe3c1 100644 --- a/tests/test_cluster_connection_pool.py +++ b/tests/test_cluster_connection_pool.py @@ -123,6 +123,13 @@ def test_get_connection_blocked(self): pool.get_connection("GET") assert unicode(ex.value).startswith("Only 'pubsub' commands can be used by get_connection()") + def test_master_node_by_slot(self): + pool = self.get_pool(connection_kwargs={}) + node = pool.get_master_node_by_slot(0) + node['port'] = 7000 + node = pool.get_master_node_by_slot(12182) + node['port'] = 7002 + class TestReadOnlyConnectionPool(object): def get_pool(self, connection_kwargs=None, max_connections=None): diff --git a/tests/test_cluster_obj.py b/tests/test_cluster_obj.py index 7855a1b9..b01d1021 100644 --- a/tests/test_cluster_obj.py +++ b/tests/test_cluster_obj.py @@ -384,13 +384,14 @@ def ok_response(connection, command_name, **options): assert p.execute() == ["MOCK_OK"] -def test_moved_redirection_on_slave_with_default_client(): - """ - Test that the client returns 'Too many Cluster redirections error' - with default (readonly_mode=False) client when we connect always to slave. - """ +def assert_moved_redirection_on_slave(sr, connection_pool_cls, cluster_obj): + + # we assume this key is set on 127.0.0.1:7000(7003) + sr.set('foo16706', 'foo') + import time + time.sleep(1) - with patch.object(ClusterConnectionPool, 'get_node_by_slot') as return_slave_mock: + with patch.object(connection_pool_cls, 'get_node_by_slot') as return_slave_mock: return_slave_mock.return_value = { 'name': '127.0.0.1:7004', 'host': '127.0.0.1', @@ -398,30 +399,62 @@ def test_moved_redirection_on_slave_with_default_client(): 'server_type': 'slave', } - normal_client = StrictRedisCluster(host="127.0.0.1", port=7000) - with pytest.raises(RedisClusterException) as ex: - normal_client.get('foo') - assert unicode(ex.value).startswith('Too many Cluster redirections') + master_value = {'host': '127.0.0.1', 'name': '127.0.0.1:7000', 'port': 7000, 'server_type': 'master'} + with patch.object( + ClusterConnectionPool, + 'get_master_node_by_slot', + return_value=master_value) as return_master_mock: + assert cluster_obj.get('foo16706') == b('foo') + assert return_master_mock.call_count == 1 + + +def test_moved_redirection_on_slave_with_default_client(sr): + """ + Test that the client is redirected normally with default + (readonly_mode=False) client even when we connect always to slave. + """ + assert_moved_redirection_on_slave( + sr, + ClusterConnectionPool, + StrictRedisCluster(host="127.0.0.1", port=7000) + ) def test_moved_redirection_on_slave_with_readonly_mode_client(sr): + """ + Ditto with READONLY mode. + """ + assert_moved_redirection_on_slave( + sr, + ClusterReadOnlyConnectionPool, + StrictRedisCluster(host="127.0.0.1", port=7000, readonly_mode=True) + ) + + +def test_access_correct_slave_with_readonly_mode_client(sr): """ Test that the client can get value normally with readonly mode - when we connect always to slave. + when we connect to correct slave. """ - # we assume this key is set on 127.0.0.1:7001 + # we assume this key is set on 127.0.0.1:7000(7003) sr.set('foo16706', 'foo') import time time.sleep(1) - with patch.object(ClusterConnectionPool, 'get_node_by_slot') as return_slave_mock: + with patch.object(ClusterReadOnlyConnectionPool, 'get_node_by_slot') as return_slave_mock: return_slave_mock.return_value = { - 'name': '127.0.0.1:7004', + 'name': '127.0.0.1:7003', 'host': '127.0.0.1', - 'port': 7004, + 'port': 7003, 'server_type': 'slave', } - readonly_client = StrictRedisCluster(host="127.0.0.1", port=7000, readonly_mode=True) - assert b('foo') == readonly_client.get('foo16706') + master_value = {'host': '127.0.0.1', 'name': '127.0.0.1:7000', 'port': 7000, 'server_type': 'master'} + with patch.object( + ClusterConnectionPool, + 'get_master_node_by_slot', + return_value=master_value) as return_master_mock: + readonly_client = StrictRedisCluster(host="127.0.0.1", port=7000, readonly_mode=True) + assert b('foo') == readonly_client.get('foo16706') + assert return_master_mock.call_count == 0 diff --git a/tests/test_pipeline.py b/tests/test_pipeline.py index 3af8d7cd..64ac5912 100644 --- a/tests/test_pipeline.py +++ b/tests/test_pipeline.py @@ -5,10 +5,13 @@ import re # rediscluster imports +from rediscluster.client import StrictRedisCluster +from rediscluster.connection import ClusterConnectionPool, ClusterReadOnlyConnectionPool from rediscluster.exceptions import RedisClusterException # 3rd party imports import pytest +from mock import patch from redis._compat import b, u, unichr, unicode from redis.exceptions import WatchError, ResponseError, ConnectionError @@ -469,3 +472,89 @@ def test_empty_stack(self, r): p = r.pipeline() result = p.execute() assert result == [] + + +class TestReadOnlyPipeline(object): + + def test_pipeline_readonly(self, r, ro): + """ + On readonly mode, we supports get related stuff only. + """ + r.set('foo71', 'a1') # we assume this key is set on 127.0.0.1:7001 + r.zadd('foo88', z1=1) # we assume this key is set on 127.0.0.1:7002 + r.zadd('foo88', z2=4) + + with ro.pipeline() as readonly_pipe: + readonly_pipe.get('foo71').zrange('foo88', 0, 5, withscores=True) + assert readonly_pipe.execute() == [ + b('a1'), + [(b('z1'), 1.0), (b('z2'), 4)], + ] + + def assert_moved_redirection_on_slave(self, connection_pool_cls, cluster_obj): + with patch.object(connection_pool_cls, 'get_node_by_slot') as return_slave_mock: + with patch.object(ClusterConnectionPool, 'get_master_node_by_slot') as return_master_mock: + def get_mock_node(role, port): + return { + 'name': '127.0.0.1:%d' % port, + 'host': '127.0.0.1', + 'port': port, + 'server_type': role, + } + + return_slave_mock.return_value = get_mock_node('slave', 7005) + return_master_mock.return_value = get_mock_node('slave', 7001) + + with cluster_obj.pipeline() as pipe: + # we assume this key is set on 127.0.0.1:7001(7004) + pipe.get('foo87').get('foo88').execute() == [None, None] + assert return_master_mock.call_count == 2 + + def test_moved_redirection_on_slave_with_default(self): + """ + On Pipeline, we redirected once and finally get from master with + readonly client when data is completely moved. + """ + self.assert_moved_redirection_on_slave( + ClusterConnectionPool, + StrictRedisCluster(host="127.0.0.1", port=7000) + ) + + def test_moved_redirection_on_slave_with_readonly_mode_client(self, sr): + """ + Ditto with READONLY mode. + """ + self.assert_moved_redirection_on_slave( + ClusterReadOnlyConnectionPool, + StrictRedisCluster(host="127.0.0.1", port=7000, readonly_mode=True) + ) + + def test_access_correct_slave_with_readonly_mode_client(self, sr): + """ + Test that the client can get value normally with readonly mode + when we connect to correct slave. + """ + + # we assume this key is set on 127.0.0.1:7001 + sr.set('foo87', 'foo') + sr.set('foo88', 'bar') + import time + time.sleep(1) + + with patch.object(ClusterReadOnlyConnectionPool, 'get_node_by_slot') as return_slave_mock: + return_slave_mock.return_value = { + 'name': '127.0.0.1:7004', + 'host': '127.0.0.1', + 'port': 7004, + 'server_type': 'slave', + } + + master_value = {'host': '127.0.0.1', 'name': '127.0.0.1:7001', 'port': 7001, 'server_type': 'master'} + with patch.object( + ClusterConnectionPool, + 'get_master_node_by_slot', + return_value=master_value) as return_master_mock: + readonly_client = StrictRedisCluster(host="127.0.0.1", port=7000, readonly_mode=True) + with readonly_client.pipeline() as readonly_pipe: + assert readonly_pipe.get('foo88').get('foo87').execute() == [b('bar'), b('foo')] + assert return_master_mock.call_count == 0 From 1ffc28e3f6137a913f8683b2910d1b97008ff39a Mon Sep 17 00:00:00 2001 From: Yoshinari Takaoka Date: Fri, 10 Jul 2015 05:53:15 +0900 Subject: [PATCH 05/50] - fixed fragile test on Docker container. --- tests/test_node_manager.py | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/tests/test_node_manager.py b/tests/test_node_manager.py index 21add47c..d98da004 100644 --- a/tests/test_node_manager.py +++ b/tests/test_node_manager.py @@ -79,20 +79,22 @@ def test_init_slots_cache(s): [10923, 16383, [b'127.0.0.1', 7002], [b'127.0.0.2', 7005]], ] - s.execute_command = lambda *args: good_slots_resp - s.connection_pool.nodes.initialize() - assert len(s.connection_pool.nodes.slots) == NodeManager.RedisClusterHashSlots - for slot_info in good_slots_resp: - all_hosts = ['127.0.0.1', '127.0.0.2'] - all_ports = [7000, 7001, 7002, 7003, 7004, 7005] - slot_start = slot_info[0] - slot_end = slot_info[1] - for i in range(slot_start, slot_end + 1): - assert len(s.connection_pool.nodes.slots[i]) == len(slot_info[2:]) - assert s.connection_pool.nodes.slots[i][0]['host'] in all_hosts - assert s.connection_pool.nodes.slots[i][1]['host'] in all_hosts - assert s.connection_pool.nodes.slots[i][0]['port'] in all_ports - assert s.connection_pool.nodes.slots[i][1]['port'] in all_ports + with patch.object(StrictRedis, 'execute_command') as execute_command_mock: + execute_command_mock.side_effect = lambda *args: good_slots_resp + + s.connection_pool.nodes.initialize() + assert len(s.connection_pool.nodes.slots) == NodeManager.RedisClusterHashSlots + for slot_info in good_slots_resp: + all_hosts = [b'127.0.0.1', b'127.0.0.2'] + all_ports = [7000, 7001, 7002, 7003, 7004, 7005] + slot_start = slot_info[0] + slot_end = slot_info[1] + for i in range(slot_start, slot_end + 1): + assert len(s.connection_pool.nodes.slots[i]) == len(slot_info[2:]) + assert s.connection_pool.nodes.slots[i][0]['host'] in all_hosts + assert s.connection_pool.nodes.slots[i][1]['host'] in all_hosts + assert s.connection_pool.nodes.slots[i][0]['port'] in all_ports + assert s.connection_pool.nodes.slots[i][1]['port'] in all_ports assert len(s.connection_pool.nodes.nodes) == 6 From 27283ba986b567a3b38a195a021302a600282553 Mon Sep 17 00:00:00 2001 From: Yoshinari Takaoka Date: Fri, 10 Jul 2015 05:58:03 +0900 Subject: [PATCH 06/50] - Added changelog item for new feature, READONLY mode. --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 509575a2..6b950613 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +* 1.1.0 + * Added READONLY mode support, scales reads using slave nodes. + * 1.0.0 * No change to anything just a bump to 1.0.0 because the lib is now considered stable/production ready. From 57a1566bd9d7c0faad394967f59a0c74c33a8592 Mon Sep 17 00:00:00 2001 From: Yoshinari Takaoka Date: Fri, 10 Jul 2015 05:59:11 +0900 Subject: [PATCH 07/50] - Added new author, mumumu. --- docs/Authors | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/Authors b/docs/Authors index 30fa9dce..ffb50f38 100644 --- a/docs/Authors +++ b/docs/Authors @@ -18,3 +18,4 @@ Authors who contributed code or testing: - 72squared - https://github.com/72squared - Neuron Teckid - https://github.com/neuront - iandyh - https://github.com/iandyh + - mumumu - https://github.com/mumumu From d9a0bbca4fe5ee85db446602b6d5e8c253d01d99 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 14 Jul 2015 20:31:21 +0200 Subject: [PATCH 08/50] Refactor exception handling - Simplified some exception handling and logic in client.py - New exception classes. They are in exceptions.py They are used in the code and can in some cases be raised to user code. - Add sleep ty TryAgainError and change what exception class is raised when TTL is exhausted to make it easier to understand what error is raised. - Update Upgrade instructions. --- CHANGES | 3 +- docs/Upgrading.md | 18 +++++++ rediscluster/client.py | 103 ++++++++++--------------------------- rediscluster/connection.py | 27 +++++++++- rediscluster/exceptions.py | 47 +++++++++++++++++ rediscluster/pipeline.py | 31 +++++------ rediscluster/utils.py | 8 +-- tests/test_cluster_obj.py | 89 ++++---------------------------- tests/test_commands.py | 4 +- tests/test_utils.py | 8 +-- 10 files changed, 154 insertions(+), 184 deletions(-) diff --git a/CHANGES b/CHANGES index 6b950613..7f3ffd64 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,5 @@ -* 1.1.0 +* Next release + * Refactored exception handling and exception classes. * Added READONLY mode support, scales reads using slave nodes. * 1.0.0 diff --git a/docs/Upgrading.md b/docs/Upgrading.md index f06f7931..615adf64 100644 --- a/docs/Upgrading.md +++ b/docs/Upgrading.md @@ -3,6 +3,24 @@ This document will describe what must be done when upgrading between different versions to ensure that code still works. +## 1.0.0 --> Next release + +The following exceptions have been changed/added and code that use this client might have to be updated to handle the new classes. + +`raise RedisClusterException("Too many Cluster redirections")` have been changed to `raise ClusterError('TTL exhausted.')` + +`ClusterDownException` have been replaced with `ClusterDownError` + +Added new `AskError` exception class. + +Added new `TryAgainError` exception class. + +Added new `MovedError` exception class. + +Added new `ClusterCrossSlotError` exception class. + + + ## 0.2.0 --> 0.3.0 In `0.3.0` release the name of the client class was changed from `RedisCluster` to `StrictRedisCluster` and a new implementation of `RedisCluster` was added that is based on `redis.Redis` class. This was done to enable implementation a cluster enabled version of `redis.Redis` class. diff --git a/rediscluster/client.py b/rediscluster/client.py index 24009f43..ce9b83ba 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -8,7 +8,10 @@ # rediscluster imports from .connection import ClusterConnectionPool, ClusterReadOnlyConnectionPool -from .exceptions import RedisClusterException, ClusterDownException +from .exceptions import ( + RedisClusterException, AskError, MovedError, ClusterDownError, + ClusterError, TryAgainError, +) from .pubsub import ClusterPubSub from .utils import ( string_keys_to_dict, @@ -128,70 +131,6 @@ def __repr__(self): servers.sort() return "{}<{}>".format(type(self).__name__, ', '.join(servers)) - def handle_cluster_command_exception(self, e): - """ - Given a exception object this method will look at the exception - message/args and determine what error was returned from redis. - - Handles: - - CLUSTERDOWN: Disconnects the connection_pool and sets - refresh_table_asap to True. - - MOVED: Updates the slots cache with the new ip:port - - ASK: Returns a dict with ip:port where to connect to try again - """ - errv = StrictRedisCluster._exception_message(e) - if errv is None: - raise e - - if errv.startswith('CLUSTERDOWN'): - # Drop all connection and reset the cluster slots/nodes - # on next attempt/command. - self.connection_pool.disconnect() - self.connection_pool.reset() - self.refresh_table_asap = True - return {'method': 'clusterdown'} - - info = self.parse_redirection_exception_msg(errv) - - if not info: - raise e - - if info['action'] == "MOVED": - self.refresh_table_asap = True - node = self.connection_pool.nodes.set_node(info['host'], info['port'], server_type='master') - self.connection_pool.nodes.slots[info['slot']][0] = node - elif info['action'] == "ASK": - node = self.connection_pool.nodes.set_node(info['host'], info['port'], server_type='master') - return {'name': node['name'], 'method': 'ask'} - - return {} - - @staticmethod - def _exception_message(e): - """ - Returns the exception message from a exception object. - - They are stored in different attributes depending on what - python version is used. - """ - errv = getattr(e, "args", None) - if not errv: - return getattr(e, "message", None) - return errv[0] - - def parse_redirection_exception_msg(self, errv): - """ - Parse `errv` exception object for MOVED or ASK error and returns - a dict with host, port and slot data about what node to talk to. - """ - errv = errv.split(" ") - - if errv[0] != "MOVED" and errv[0] != "ASK": - return None - - a = errv[2].split(":") - return {"action": errv[0], "slot": int(errv[1]), "host": a[0], "port": int(a[1])} - def pubsub(self, **kwargs): return ClusterPubSub(self.connection_pool, **kwargs) @@ -272,18 +211,21 @@ def execute_command(self, *args, **kwargs): self.connection_pool.nodes.initialize() self.refresh_table_asap = False - action = {} + redirect_addr = None + asking = False + command = args[0] try_random_node = False slot = self._determine_slot(*args) ttl = int(self.RedisClusterRequestTTL) + while ttl > 0: ttl -= 1 - if action.get("method", "") == "ask": - node = self.connection_pool.nodes.nodes[action['name']] + + if asking: + node = self.connection_pool.nodes.nodes[redirect_addr] r = self.connection_pool.get_connection_by_node(node) elif try_random_node: - # TODO: Untested r = self.connection_pool.get_random_connection() try_random_node = False else: @@ -296,21 +238,32 @@ def execute_command(self, *args, **kwargs): try: r.send_command(*args) return self.parse_response(r, command, **kwargs) - except (RedisClusterException, BusyLoadingError): raise except (ConnectionError, TimeoutError): try_random_node = True if ttl < self.RedisClusterRequestTTL / 2: time.sleep(0.1) - except ResponseError as e: - action = self.handle_cluster_command_exception(e) - if action.get("method", "") == "clusterdown": - raise ClusterDownException() + except ClusterDownError as e: + self.connection_pool.disconnect() + self.connection_pool.reset() + self.refresh_table_asap = True + raise e + except MovedError as e: + self.refresh_table_asap = True + node = self.connection_pool.nodes.set_node(e.host, e.port, server_type='master') + self.connection_pool.nodes.slots[e.slot_id][0] = node + redirect_addr = "%s:%s" % (e.host, e.port) + except TryAgainError as e: + if ttl < self.COMMAND_TTL / 2: + time.sleep(0.05) + except AskError as e: + node = self.connection_pool.nodes.set_node(e.host, e.port, server_type='master') + redirect_addr, asking = "%s:%s" % (e.host, e.port), True finally: self.connection_pool.release(r) - raise RedisClusterException("Too many Cluster redirections") + raise ClusterError('TTL exhausted.') def _execute_command_on_nodes(self, nodes, *args, **kwargs): command = args[0] diff --git a/rediscluster/connection.py b/rediscluster/connection.py index f0f48e23..5fb7c7a3 100644 --- a/rediscluster/connection.py +++ b/rediscluster/connection.py @@ -9,23 +9,46 @@ # rediscluster imports from .nodemanager import NodeManager -from .exceptions import RedisClusterException +from .exceptions import ( + RedisClusterException, AskError, MovedError, + TryAgainError, ClusterDownError, ClusterCrossSlotError, +) # 3rd party imports from redis._compat import nativestr -from redis.connection import ConnectionPool, Connection +from redis.client import dict_merge +from redis.connection import ConnectionPool, Connection, DefaultParser from redis.exceptions import ConnectionError +class ClusterParser(DefaultParser): + EXCEPTION_CLASSES = dict_merge( + DefaultParser.EXCEPTION_CLASSES, { + 'ASK': AskError, + 'TRYAGAIN': TryAgainError, + 'MOVED': MovedError, + 'CLUSTERDOWN': ClusterDownError, + 'CROSSSLOT': ClusterCrossSlotError, + }) + + class ClusterConnection(Connection): "Manages TCP communication to and from a Redis server" description_format = "ClusterConnection" + def __init__(self, *args, **kwargs): + kwargs['parser_class'] = ClusterParser + super(ClusterConnection, self).__init__(*args, **kwargs) + class ClusterReadOnlyConnection(Connection): "Manages READONLY TCP communication to and from a Redis server" description_format = "ClusterReadOnlyConnection" + def __init__(self, *args, **kwargs): + kwargs['parser_class'] = ClusterParser + super(ClusterReadOnlyConnection, self).__init__(*args, **kwargs) + def on_connect(self): "Initialize the connection, authenticate and select a database and send READONLY command" super(ClusterReadOnlyConnection, self).on_connect() diff --git a/rediscluster/exceptions.py b/rediscluster/exceptions.py index a07efbe9..3c29499a 100644 --- a/rediscluster/exceptions.py +++ b/rediscluster/exceptions.py @@ -1,5 +1,9 @@ # -*- coding: utf-8 -*- +from redis.exceptions import ( + ResponseError, RedisError, +) + class RedisClusterException(Exception): pass @@ -11,3 +15,46 @@ class RedisClusterError(Exception): class ClusterDownException(Exception): pass + + +class ClusterError(RedisError): + pass + + +class ClusterCrossSlotError(ResponseError): + message = "Keys in request don't hash to the same slot" + + +class ClusterDownError(ClusterError, ResponseError): + def __init__(self, resp): + self.args = (resp, ) + self.message = resp + + +class AskError(ResponseError): + """ + src node: MIGRATING to dst node + get > ASK error + ask dst node > ASKING command + dst node: IMPORTING from src node + asking command only affects next command + any op will be allowed after asking command + """ + + def __init__(self, resp): + """should only redirect to master node""" + self.args = (resp, ) + self.message = resp + slot_id, new_node = resp.split(' ') + host, port = new_node.rsplit(':', 1) + self.slot_id = int(slot_id) + self.node_addr = self.host, self.port = host, int(port) + + +class TryAgainError(ResponseError): + def __init__(self, resp): + pass + + +class MovedError(AskError): + pass diff --git a/rediscluster/pipeline.py b/rediscluster/pipeline.py index 73b8a7a8..d9ac7fb0 100644 --- a/rediscluster/pipeline.py +++ b/rediscluster/pipeline.py @@ -9,7 +9,9 @@ # rediscluster imports from .client import StrictRedisCluster from .connection import by_node_context -from .exceptions import RedisClusterException, ClusterDownException +from .exceptions import ( + RedisClusterException, AskError, MovedError, ClusterDownError, +) from .utils import clusterdown_wrapper # 3rd party imports @@ -196,30 +198,23 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T time.sleep(0.1) continue - errv = StrictRedisCluster._exception_message(v) - if errv is None: - continue - - if errv.startswith('CLUSTERDOWN'): + # If cluster is down it should be raised and bubble up to + # utils.clusterdown_wrapper() + if isinstance(v, ClusterDownError): self.connection_pool.disconnect() self.connection_pool.reset() self.refresh_table_asap = True - raise ClusterDownException() - - redir = self.parse_redirection_exception_msg(errv) - - if not redir: - continue + raise v - if redir['action'] == "MOVED": + if isinstance(v, MovedError): self.refresh_table_asap = True - node = self.connection_pool.nodes.set_node(redir['host'], redir['port'], server_type='master') - self.connection_pool.nodes.slots[redir['slot']][0] = node + node = self.connection_pool.nodes.set_node(v.host, v.port, server_type='master') + self.connection_pool.nodes.slots[v.slot_id][0] = node attempt.append(i) self._fail_on_redirect(allow_redirections) - elif redir['action'] == "ASK": - node = self.connection_pool.nodes.set_node(redir['host'], redir['port'], server_type='master') - ask_slots[redir['slot']] = node + elif isinstance(v, AskError): + node = self.connection_pool.nodes.set_node(v.host, v.port, server_type='master') + ask_slots[v.slot_id] = node attempt.append(i) self._fail_on_redirect(allow_redirections) diff --git a/rediscluster/utils.py b/rediscluster/utils.py index 616d8be3..36468390 100644 --- a/rediscluster/utils.py +++ b/rediscluster/utils.py @@ -2,7 +2,9 @@ from socket import gethostbyaddr # rediscluster imports -from .exceptions import RedisClusterException, ClusterDownException +from .exceptions import ( + RedisClusterException, ClusterDownError +) def is_dict(d): @@ -83,13 +85,13 @@ def inner(*args, **kwargs): for _ in range(0, 3): try: return func(*args, **kwargs) - except ClusterDownException: + except ClusterDownError: # Try again with the new cluster setup. All other errors # should be raised. pass # If it fails 3 times then raise exception back to caller - raise ClusterDownException("CLUSTERDOWN error. Unable to rebuild the cluster") + raise ClusterDownError("CLUSTERDOWN error. Unable to rebuild the cluster") return inner diff --git a/tests/test_cluster_obj.py b/tests/test_cluster_obj.py index b01d1021..09a2a8e9 100644 --- a/tests/test_cluster_obj.py +++ b/tests/test_cluster_obj.py @@ -7,17 +7,17 @@ # rediscluster imports from rediscluster import StrictRedisCluster from rediscluster.connection import ClusterConnectionPool, ClusterReadOnlyConnectionPool -from rediscluster.exceptions import RedisClusterException +from rediscluster.exceptions import ( + RedisClusterException, MovedError, AskError, ClusterDownError, +) from rediscluster.nodemanager import NodeManager from tests.conftest import _get_client, skip_if_server_version_lt # 3rd party imports from mock import patch, Mock -from redis.exceptions import ResponseError from redis._compat import b, unicode import pytest - pytestmark = skip_if_server_version_lt('2.9.0') @@ -111,10 +111,7 @@ def ok_call(self, *args, **kwargs): return "OK" parse_response_mock.side_effect = ok_call - resp = ResponseError() - resp.args = ('CLUSTERDOWN The cluster is down. Use CLUSTER INFO for more information',) - resp.message = 'CLUSTERDOWN The cluster is down. Use CLUSTER INFO for more information' - raise resp + raise ClusterDownError('CLUSTERDOWN The cluster is down. Use CLUSTER INFO for more information') def side_effect_rebuild_slots_cache(self): # make new node cache that points to 7007 instead of 7006 @@ -163,66 +160,6 @@ def map_7007(self): p.execute() -def test_moved_exception_handling(r): - """ - Test that `handle_cluster_command_exception` deals with MOVED - error correctly. - """ - resp = ResponseError() - resp.message = "MOVED 1337 127.0.0.1:7000" - r.handle_cluster_command_exception(resp) - assert r.refresh_table_asap is True - assert r.connection_pool.nodes.slots[1337][0] == { - "host": "127.0.0.1", - "port": 7000, - "name": "127.0.0.1:7000", - "server_type": "master", - } - - -def test_ask_exception_handling(r): - """ - Test that `handle_cluster_command_exception` deals with ASK - error correctly. - """ - resp = ResponseError() - resp.message = "ASK 1337 127.0.0.1:7000" - assert r.handle_cluster_command_exception(resp) == { - "name": "127.0.0.1:7000", - "method": "ask", - } - - -def test_raise_regular_exception(r): - """ - If a non redis server exception is passed in it shold be - raised again. - """ - e = Exception("foobar") - with pytest.raises(Exception) as ex: - r.handle_cluster_command_exception(e) - assert unicode(ex.value).startswith("foobar") - - -def test_clusterdown_exception_handling(): - """ - Test that if exception message starts with CLUSTERDOWN it should - disconnect the connection pool and set refresh_table_asap to True. - """ - with patch.object(ClusterConnectionPool, 'disconnect') as mock_disconnect: - with patch.object(ClusterConnectionPool, 'reset') as mock_reset: - r = StrictRedisCluster(host="127.0.0.1", port=7000) - i = len(mock_reset.mock_calls) - - assert r.handle_cluster_command_exception(Exception("CLUSTERDOWN")) == {"method": "clusterdown"} - assert r.refresh_table_asap is True - - mock_disconnect.assert_called_once_with() - - # reset() should only be called once inside `handle_cluster_command_exception` - assert len(mock_reset.mock_calls) - i == 1 - - def test_execute_command_errors(r): """ If no command is given to `_determine_nodes` then exception @@ -281,9 +218,7 @@ def ok_response(connection, command_name, **options): return "MOCK_OK" m.side_effect = ok_response - resp = ResponseError() - resp.message = "ASK 1337 127.0.0.1:7001" - raise resp + raise AskError("1337 127.0.0.1:7001") m.side_effect = ask_redirect_effect @@ -291,7 +226,7 @@ def ok_response(connection, command_name, **options): assert r.execute_command("SET", "foo", "bar") == "MOCK_OK" -def test_ask_redirection_pipeline(): +def test_pipeline_ask_redirection(): """ Test that the server handles ASK response when used in pipeline. @@ -312,9 +247,7 @@ def ok_response(connection, command_name, **options): return "MOCK_OK" m.side_effect = ok_response - resp = ResponseError() - resp.message = "ASK 12182 127.0.0.1:7001" - raise resp + raise AskError("12182 127.0.0.1:7001") m.side_effect = ask_redirect_effect @@ -342,9 +275,7 @@ def ok_response(connection, command_name, **options): return "MOCK_OK" m.side_effect = ok_response - resp = ResponseError() - resp.message = "MOVED 12182 127.0.0.1:7002" - raise resp + raise MovedError("12182 127.0.0.1:7002") m.side_effect = ask_redirect_effect @@ -373,9 +304,7 @@ def ok_response(connection, command_name, **options): return "MOCK_OK" m.side_effect = ok_response - resp = ResponseError() - resp.message = "MOVED 12182 127.0.0.1:7002" - raise resp + raise MovedError("12182 127.0.0.1:7002") m.side_effect = moved_redirect_effect diff --git a/tests/test_commands.py b/tests/test_commands.py index e29aa58d..24ea5399 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -806,7 +806,7 @@ def test_zinterstore_fail_cross_slot(self, r): r.zadd('c', a1=6, a3=5, a4=4) with pytest.raises(ResponseError) as excinfo: r.zinterstore('d', ['a', 'b', 'c']) - assert re.search('CROSSSLOT', str(excinfo)) + assert re.search('ClusterCrossSlotError', str(excinfo)) def test_zinterstore_sum(self, r): r.zadd('a{foo}', a1=1, a2=1, a3=1) @@ -971,7 +971,7 @@ def test_zunionstore_fail_crossslot(self, r): r.zadd('c', a1=6, a3=5, a4=4) with pytest.raises(ResponseError) as excinfo: r.zunionstore('d', ['a', 'b', 'c']) - assert re.search('CROSSSLOT', str(excinfo)) + assert re.search('ClusterCrossSlotError', str(excinfo)) def test_zunionstore_sum(self, r): r.zadd('a{foo}', a1=1, a2=1, a3=1) diff --git a/tests/test_utils.py b/tests/test_utils.py index a2fa64fc..0fbe40d2 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -4,7 +4,9 @@ from __future__ import with_statement # rediscluster imports -from rediscluster.exceptions import RedisClusterException, ClusterDownException +from rediscluster.exceptions import ( + RedisClusterException, ClusterDownError +) from rediscluster.utils import ( string_keys_to_dict, dict_merge, @@ -62,8 +64,8 @@ def test_first_key(): def test_clusterdown_wrapper(): @clusterdown_wrapper def bad_func(): - raise ClusterDownException() + raise ClusterDownError("CLUSTERDOWN") - with pytest.raises(ClusterDownException) as cex: + with pytest.raises(ClusterDownError) as cex: bad_func() assert unicode(cex.value).startswith("CLUSTERDOWN error. Unable to rebuild the cluster") From 63ddbc7dca9a74b11b2462b193a4dca2a5253fb3 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Mon, 20 Jul 2015 19:26:35 +0200 Subject: [PATCH 09/50] Fix __repr__ for ClusterConnectionPool and ClusterReadOnlyConnectionPool --- CHANGES | 1 + rediscluster/connection.py | 6 ++++- tests/test_cluster_connection_pool.py | 35 +++++++++++++++++++++------ 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/CHANGES b/CHANGES index 7f3ffd64..3d17a791 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,7 @@ * Next release * Refactored exception handling and exception classes. * Added READONLY mode support, scales reads using slave nodes. + * Fix __repr__ for ClusterConnectionPool and ClusterReadOnlyConnectionPool * 1.0.0 * No change to anything just a bump to 1.0.0 because the lib is now considered stable/production ready. diff --git a/rediscluster/connection.py b/rediscluster/connection.py index 5fb7c7a3..cc2a34b5 100644 --- a/rediscluster/connection.py +++ b/rediscluster/connection.py @@ -83,9 +83,13 @@ def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=Cl self.connection_kwargs["socket_timeout"] = ClusterConnectionPool.RedisClusterDefaultTimeout def __repr__(self): + """ + Return a string with all unique ip:port combinations that this pool is connected to. + """ + nodes = [{'host': i['host'], 'port': i['port']} for i in self.nodes.startup_nodes] return "%s<%s>" % ( type(self).__name__, - self.connection_class.description_format % self.connection_kwargs, + ", ".join([self.connection_class.description_format % dict(node, **self.connection_kwargs) for node in nodes]) ) def reset(self): diff --git a/tests/test_cluster_connection_pool.py b/tests/test_cluster_connection_pool.py index de9fe3c1..245acdb0 100644 --- a/tests/test_cluster_connection_pool.py +++ b/tests/test_cluster_connection_pool.py @@ -33,12 +33,13 @@ def __init__(self, host="localhost", port=7000, socket_timeout=None, **kwargs): class TestConnectionPool(object): - def get_pool(self, connection_kwargs=None, max_connections=None, connection_class=DummyConnection): + def get_pool(self, connection_kwargs=None, max_connections=None, connection_class=DummyConnection, init_slot_cache=True): connection_kwargs = connection_kwargs or {} pool = ClusterConnectionPool( connection_class=connection_class, max_connections=max_connections, startup_nodes=[{"host": "127.0.0.1", "port": 7000}], + init_slot_cache=init_slot_cache, **connection_kwargs) return pool @@ -70,16 +71,26 @@ def test_reuse_previously_released_connection(self): assert c1 == c2 def test_repr_contains_db_info_tcp(self): + """ + Note: init_slot_cache muts be set to false otherwise it will try to + query the test server for data and then it can't be predicted reliably + """ connection_kwargs = {'host': 'localhost', 'port': 7000} pool = self.get_pool(connection_kwargs=connection_kwargs, - connection_class=ClusterConnection) + connection_class=ClusterConnection, + init_slot_cache=False) expected = 'ClusterConnectionPool>' assert repr(pool) == expected def test_repr_contains_db_info_unix(self): + """ + Note: init_slot_cache muts be set to false otherwise it will try to + query the test server for data and then it can't be predicted reliably + """ connection_kwargs = {'path': '/abc', 'db': 1} pool = self.get_pool(connection_kwargs=connection_kwargs, - connection_class=UnixDomainSocketConnection) + connection_class=UnixDomainSocketConnection, + init_slot_cache=False) expected = 'ClusterConnectionPool>' assert repr(pool) == expected @@ -132,18 +143,26 @@ def test_master_node_by_slot(self): class TestReadOnlyConnectionPool(object): - def get_pool(self, connection_kwargs=None, max_connections=None): + def get_pool(self, connection_kwargs=None, max_connections=None, init_slot_cache=True, startup_nodes=None): + startup_nodes = startup_nodes or [{'host': '127.0.0.1', 'port': 7000}] connection_kwargs = connection_kwargs or {} pool = ClusterReadOnlyConnectionPool( + init_slot_cache=init_slot_cache, max_connections=max_connections, - startup_nodes=[{"host": "127.0.0.1", "port": 7000}], + startup_nodes=startup_nodes, **connection_kwargs) return pool def test_repr_contains_db_info_readonly(self): - connection_kwargs = {'host': 'localhost', 'port': 7000} - pool = self.get_pool(connection_kwargs=connection_kwargs) - expected = 'ClusterReadOnlyConnectionPool>' + """ + Note: init_slot_cache must be set to false otherwise it will try to + query the test server for data and then it can't be predicted reliably + """ + pool = self.get_pool( + init_slot_cache=False, + startup_nodes=[{"host": "127.0.0.1", "port": 7000}, {"host": "127.0.0.2", "port": 7001}], + ) + expected = 'ClusterReadOnlyConnectionPool, ClusterReadOnlyConnection>' assert repr(pool) == expected def test_max_connections(self): From aef594b6614b3bef48aecad92a6dc49fc0760376 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Mon, 20 Jul 2015 22:21:50 +0200 Subject: [PATCH 10/50] Add link to new google group topic about pubsub scaling issues --- docs/Pubsub.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/Pubsub.md b/docs/Pubsub.md index 1b43195e..293e79de 100644 --- a/docs/Pubsub.md +++ b/docs/Pubsub.md @@ -2,7 +2,7 @@ After testing pubsub in cluster mode one big problem was discovered with the `PUBLISH` command. -According to the current official redis documentation on `PUBLISH` "Integer reply: the number of clients that received the message." it was initially assumed that if we had clients connected to different nodes in the cluster it would still report back the correct number of clients that recieved the message. +According to the current official redis documentation on `PUBLISH` "Integer reply: the number of clients that received the message." it was initially assumed that if we had clients connected to different nodes in the cluster it would still report back the correct number of clients that recieved the message. However after some testing of this command it was discovered that it would only report the number of clients that have subscribed on the same server the `PUBLISH` command was executed on. @@ -18,6 +18,14 @@ Discussion on this topic can be found here: https://groups.google.com/forum/?hl= +# Scalability issues + +The following part is from this discussion https://groups.google.com/forum/?hl=sv#!topic/redis-db/B0_fvfDWLGM and it describes the scalability issue that pubsub has and the performance that goes with it when used in a cluster environment. + + according to [1] and [2] PubSub works by broadcasting every publish to every other Redis Cluster node. This limits the PubSub throughput to the bisection bandwidth of the underlying network infrastructure divided by the number of nodes times message size. So if a typical message has 1KB, the cluster has 10 nodes and bandwidth is 1 GBit/s, throughput is already limited to 12.5K RPS. If we increase the message size to 5 KB and the number of nodes to 50, we only get 500 RPS - much less than a single Redis instance could service (>100K RPS), while putting maximum pressure on the network. PubSub thus scales linearly wrt. to the cluster size, but in the the negative direction! + + + # How pubsub works in StrictRedisCluster In `0.2.0` a first solution to pubsub problem was implemented, but it contains some limitations. From cc1f1a6221c537e9d4b94c18ffbc9dca673b1484 Mon Sep 17 00:00:00 2001 From: Grok Date: Mon, 20 Jul 2015 22:24:29 +0200 Subject: [PATCH 11/50] Reformat text --- docs/Pubsub.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/Pubsub.md b/docs/Pubsub.md index 293e79de..97ca35be 100644 --- a/docs/Pubsub.md +++ b/docs/Pubsub.md @@ -22,7 +22,15 @@ Discussion on this topic can be found here: https://groups.google.com/forum/?hl= The following part is from this discussion https://groups.google.com/forum/?hl=sv#!topic/redis-db/B0_fvfDWLGM and it describes the scalability issue that pubsub has and the performance that goes with it when used in a cluster environment. - according to [1] and [2] PubSub works by broadcasting every publish to every other Redis Cluster node. This limits the PubSub throughput to the bisection bandwidth of the underlying network infrastructure divided by the number of nodes times message size. So if a typical message has 1KB, the cluster has 10 nodes and bandwidth is 1 GBit/s, throughput is already limited to 12.5K RPS. If we increase the message size to 5 KB and the number of nodes to 50, we only get 500 RPS - much less than a single Redis instance could service (>100K RPS), while putting maximum pressure on the network. PubSub thus scales linearly wrt. to the cluster size, but in the the negative direction! + according to [1] and [2] PubSub works by broadcasting every publish to every other + Redis Cluster node. This limits the PubSub throughput to the bisection bandwidth + of the underlying network infrastructure divided by the number of nodes times + message size. So if a typical message has 1KB, the cluster has 10 nodes and + bandwidth is 1 GBit/s, throughput is already limited to 12.5K RPS. If we increase + the message size to 5 KB and the number of nodes to 50, we only get 500 RPS + much less than a single Redis instance could service (>100K RPS), while putting + maximum pressure on the network. PubSub thus scales linearly wrt. to the cluster size, + but in the the negative direction! From f59ade618952d66b175e881667a3888c3e6bf038 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Mon, 20 Jul 2015 23:02:06 +0200 Subject: [PATCH 12/50] Merged ClusterReadOnlyConnection with ClusterConnection --- rediscluster/connection.py | 27 ++++++++++++--------------- tests/test_cluster_connection_pool.py | 2 +- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/rediscluster/connection.py b/rediscluster/connection.py index cc2a34b5..8916600d 100644 --- a/rediscluster/connection.py +++ b/rediscluster/connection.py @@ -37,25 +37,21 @@ class ClusterConnection(Connection): description_format = "ClusterConnection" def __init__(self, *args, **kwargs): + self.readonly = kwargs.pop('readonly', False) kwargs['parser_class'] = ClusterParser super(ClusterConnection, self).__init__(*args, **kwargs) - -class ClusterReadOnlyConnection(Connection): - "Manages READONLY TCP communication to and from a Redis server" - description_format = "ClusterReadOnlyConnection" - - def __init__(self, *args, **kwargs): - kwargs['parser_class'] = ClusterParser - super(ClusterReadOnlyConnection, self).__init__(*args, **kwargs) - def on_connect(self): - "Initialize the connection, authenticate and select a database and send READONLY command" - super(ClusterReadOnlyConnection, self).on_connect() + ''' + Initialize the connection, authenticate and select a database and send READONLY if it is + set during object initialization. + ''' + super(ClusterConnection, self).on_connect() - self.send_command('READONLY') - if nativestr(self.read_response()) != 'OK': - raise ConnectionError('READONLY command failed') + if self.readonly: + self.send_command('READONLY') + if nativestr(self.read_response()) != 'OK': + raise ConnectionError('READONLY command failed') class UnixDomainSocketConnection(Connection): @@ -252,12 +248,13 @@ class ClusterReadOnlyConnectionPool(ClusterConnectionPool): Readonly connection pool for rediscluster """ - def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=ClusterReadOnlyConnection, max_connections=None, **connection_kwargs): + def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=ClusterConnection, max_connections=None, **connection_kwargs): super(ClusterReadOnlyConnectionPool, self).__init__( startup_nodes=startup_nodes, init_slot_cache=init_slot_cache, connection_class=connection_class, max_connections=max_connections, + readonly=True, **connection_kwargs) def get_node_by_slot(self, slot): diff --git a/tests/test_cluster_connection_pool.py b/tests/test_cluster_connection_pool.py index 245acdb0..a605ef8a 100644 --- a/tests/test_cluster_connection_pool.py +++ b/tests/test_cluster_connection_pool.py @@ -162,7 +162,7 @@ def test_repr_contains_db_info_readonly(self): init_slot_cache=False, startup_nodes=[{"host": "127.0.0.1", "port": 7000}, {"host": "127.0.0.2", "port": 7001}], ) - expected = 'ClusterReadOnlyConnectionPool, ClusterReadOnlyConnection>' + expected = 'ClusterReadOnlyConnectionPool, ClusterConnection>' assert repr(pool) == expected def test_max_connections(self): From 27156f443e384acb99b70e8757b1d5e2fdc080e0 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Mon, 20 Jul 2015 23:21:21 +0200 Subject: [PATCH 13/50] Enable container support for travis --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 175e9353..06598716 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,3 +1,4 @@ +sudo: false language: python python: - "3.3" From e77a82962224dbdae9e9a2b808d94c365498936c Mon Sep 17 00:00:00 2001 From: Aaron Westendorf Date: Wed, 5 Aug 2015 17:00:36 -0400 Subject: [PATCH 14/50] Add max_connections_per_node on ClusterConnectionPool This handles a situation where the max_connections argument applies to the whole cluster, and can starve connections on concurrent processes to nodes in the cluster. It also results in asymmetrical connection allocation across the nodes, which makes it challenging to configure file descriptor and redis max client settings. The cluster-wide setting can also cause problems if the number of nodes is greater than max_connections. --- rediscluster/connection.py | 11 ++++++++--- tests/test_cluster_connection_pool.py | 12 +++++++++++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/rediscluster/connection.py b/rediscluster/connection.py index 8916600d..449129af 100644 --- a/rediscluster/connection.py +++ b/rediscluster/connection.py @@ -64,9 +64,11 @@ class ClusterConnectionPool(ConnectionPool): """ RedisClusterDefaultTimeout = None - def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=ClusterConnection, max_connections=None, **connection_kwargs): + def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=ClusterConnection, max_connections=None, max_connections_per_node=False, **connection_kwargs): super(ClusterConnectionPool, self).__init__(connection_class=connection_class, max_connections=max_connections) + self.max_connections_per_node = max_connections_per_node + self.nodes = NodeManager(startup_nodes) if init_slot_cache: self.nodes.initialize() @@ -128,7 +130,7 @@ def make_connection(self, node): """ Create a new connection """ - if self.count_num_connections() >= self.max_connections: + if self.count_num_connections(node) >= self.max_connections: raise RedisClusterException("Too many connections") self._created_connections += 1 connection = self.connection_class(host=node["host"], port=node["port"], **self.connection_kwargs) @@ -183,7 +185,10 @@ def disconnect(self): for connection in all_pubsub_conns: connection.disconnect() - def count_num_connections(self): + def count_num_connections(self, node): + if self.max_connections_per_node: + return len(self._in_use_connections.get(node['name'], [])) + i = 0 for _, connections in self._in_use_connections.items(): i += len(connections) diff --git a/tests/test_cluster_connection_pool.py b/tests/test_cluster_connection_pool.py index a605ef8a..7e569612 100644 --- a/tests/test_cluster_connection_pool.py +++ b/tests/test_cluster_connection_pool.py @@ -33,11 +33,12 @@ def __init__(self, host="localhost", port=7000, socket_timeout=None, **kwargs): class TestConnectionPool(object): - def get_pool(self, connection_kwargs=None, max_connections=None, connection_class=DummyConnection, init_slot_cache=True): + def get_pool(self, connection_kwargs=None, max_connections=None, max_connections_per_node=None, connection_class=DummyConnection, init_slot_cache=True): connection_kwargs = connection_kwargs or {} pool = ClusterConnectionPool( connection_class=connection_class, max_connections=max_connections, + max_connections_per_node=max_connections_per_node, startup_nodes=[{"host": "127.0.0.1", "port": 7000}], init_slot_cache=init_slot_cache, **connection_kwargs) @@ -63,6 +64,15 @@ def test_max_connections(self): with pytest.raises(RedisClusterException): pool.get_connection_by_node({"host": "127.0.0.1", "port": 7000}) + def test_max_connections_per_node(self): + pool = self.get_pool(max_connections=2, max_connections_per_node=True) + pool.get_connection_by_node({"host": "127.0.0.1", "port": 7000}) + pool.get_connection_by_node({"host": "127.0.0.1", "port": 7001}) + pool.get_connection_by_node({"host": "127.0.0.1", "port": 7000}) + pool.get_connection_by_node({"host": "127.0.0.1", "port": 7001}) + with pytest.raises(RedisClusterException): + pool.get_connection_by_node({"host": "127.0.0.1", "port": 7000}) + def test_reuse_previously_released_connection(self): pool = self.get_pool() c1 = pool.get_connection_by_node({"host": "127.0.0.1", "port": 7000}) From 848a3477fa455d81d75071421bb7fd96b7c0bcaa Mon Sep 17 00:00:00 2001 From: Aaron Westendorf Date: Thu, 6 Aug 2015 11:14:52 -0400 Subject: [PATCH 15/50] Documentation of max_connections_per_node changes --- CHANGES | 1 + docs/Authors | 1 + docs/Upgrading.md | 2 ++ 3 files changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index 3d17a791..4f7429b7 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,7 @@ * Refactored exception handling and exception classes. * Added READONLY mode support, scales reads using slave nodes. * Fix __repr__ for ClusterConnectionPool and ClusterReadOnlyConnectionPool + * Add max_connections_per_node parameter to ClusterConnectionPool so that max_connections parameter is calculated per-node rather than across the whole cluster. * 1.0.0 * No change to anything just a bump to 1.0.0 because the lib is now considered stable/production ready. diff --git a/docs/Authors b/docs/Authors index ffb50f38..3ccb0bb4 100644 --- a/docs/Authors +++ b/docs/Authors @@ -19,3 +19,4 @@ Authors who contributed code or testing: - Neuron Teckid - https://github.com/neuront - iandyh - https://github.com/iandyh - mumumu - https://github.com/mumumu + - awestendorf - https://github.com/awestendorf diff --git a/docs/Upgrading.md b/docs/Upgrading.md index 615adf64..7401cd37 100644 --- a/docs/Upgrading.md +++ b/docs/Upgrading.md @@ -19,6 +19,8 @@ Added new `MovedError` exception class. Added new `ClusterCrossSlotError` exception class. +Added optional `max_connections_per_node` parameter to `ClusterConnectionPool` which changes behavior of `max_connections` so that it applies per-node rather than across the whole cluster. The new feature is opt-in, and the existing default behavior is unchanged. Users are recommended to opt-in as the feature fixes two important problems. First is that some nodes could be starved for connections after max_connections is used up by connecting to other nodes. Second is that the asymmetric number of connections across nodes makes it challenging to configure file descriptor and redis max client settings. + ## 0.2.0 --> 0.3.0 From 2d6b8fe752e4e4dc436e1c117486511623f10a09 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Wed, 12 Aug 2015 10:04:32 +0200 Subject: [PATCH 16/50] Add python 3.5 testing in travis-ci and tox. Refactor travis-ci and tox script. --- .travis.yml | 19 +++++++++++++++++-- README.md | 3 +++ dev-requirements.txt | 5 +++-- tox.ini | 42 ++++++++++++++++-------------------------- 4 files changed, 39 insertions(+), 30 deletions(-) diff --git a/.travis.yml b/.travis.yml index 06598716..1bb1e619 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,14 +1,29 @@ sudo: false language: python python: + - "2.7" + - "3.2" - "3.3" + - "3.4" + - "3.5-dev" services: - redis-server install: - make redis-install - - pip install coverage python-coveralls tox + - pip install -r dev-requirements.txt - pip install -e . + - "if [[ $TEST_HIREDIS == '1' ]]; then pip install hiredis; fi" +env: + - TEST_HIREDIS=0 + - TEST_HIREDIS=1 script: - - make test + - make start + - coverage erase + - coverage run --source rediscluster -p -m py.test + - make stop after_success: + - coverage combine - coveralls +matrix: + allow_failures: + - python: "3.5-dev" diff --git a/README.md b/README.md index 0b2b747d..0d06af8c 100644 --- a/README.md +++ b/README.md @@ -38,9 +38,12 @@ Supported python versions, all minor releases in each major version should be su - 3.2.x - 3.3.x - 3.4.1+ +- 3.5.0 (Beta 4) Python 3.4.0 do not not work with pubsub because of segfault issues (Same as redis-py has). If rediscluster is runned on 3.4.0 it will raise RuntimeError exception and exit. If you get this error locally when running tox, consider using `pyenv` to fix this problem. +Testing with python 3.5.0 Beta 4 on travis-ci to ensure it will work when it is released in a stable version. + ## Installation diff --git a/dev-requirements.txt b/dev-requirements.txt index 4946aada..f9ae0415 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,8 +1,9 @@ -r requirements.txt coverage >= 3.7.1 -hiredis >= 0.1.3 -pytest >= 2.5.0 +-e git://github.com/pytest-dev/pytest.git@master#egg=pytest testfixtures >= 4.0.1 mock == 1.0.1 docopt == 0.6.2 +tox +python-coveralls diff --git a/tox.ini b/tox.ini index 9addb9d8..b355bf6b 100644 --- a/tox.ini +++ b/tox.ini @@ -4,51 +4,41 @@ # install tox" and then run "tox" from this directory. [tox] -envlist = py27, py32, py33, py34, hi27, hi32, hi33, hi34, flake8-py34, flake8-py27 +envlist = py27, py32, py33, py34, py35, hi27, hi32, hi33, hi34, hi35, flake8-py34, flake8-py27 [testenv] -deps = - coverage == 3.7.1 - pytest >= 2.5.0 - testfixtures == 4.0.1 - mock == 1.0.1 +deps = -r{toxinidir}/dev-requirements.txt commands = python {envbindir}/coverage run --source rediscluster -p -m py.test [testenv:hi27] basepython = python2.7 deps = - coverage == 3.7.1 - hiredis >= 0.1.3 - pytest >= 2.5.0 - testfixtures == 4.0.1 - mock == 1.0.1 + -r{toxinidir}/dev-requirements.txt + hiredis == 0.2.0 [testenv:hi32] basepython = python3.2 deps = - coverage == 3.7.1 - hiredis >= 0.1.3 - pytest >= 2.5.0 - testfixtures == 4.0.1 - mock == 1.0.1 + -r{toxinidir}/dev-requirements.txt + hiredis == 0.2.0 [testenv:hi33] basepython = python3.3 deps = - coverage == 3.7.1 - hiredis >= 0.1.3 - pytest >= 2.5.0 - testfixtures == 4.0.1 - mock == 1.0.1 + -r{toxinidir}/dev-requirements.txt + hiredis == 0.2.0 [testenv:hi34] basepython = python3.4 deps = - coverage == 3.7.1 - hiredis >= 0.1.3 - pytest >= 2.5.0 - testfixtures == 4.0.1 - mock == 1.0.1 + -r{toxinidir}/dev-requirements.txt + hiredis == 0.2.0 + +[testenv:hi35] +basepython = python3.5 +deps = + -r{toxinidir}/dev-requirements.txt + hiredis == 0.2.0 [testenv:flake8-py34] basepython= python3.4 From 9b22b332da5dc36d91faf5a2117d9faeabf74b20 Mon Sep 17 00:00:00 2001 From: iandyh Date: Mon, 7 Sep 2015 16:55:54 +0900 Subject: [PATCH 17/50] check pid before getting connection. Make fork safe --- rediscluster/connection.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rediscluster/connection.py b/rediscluster/connection.py index 449129af..2a8fff6f 100644 --- a/rediscluster/connection.py +++ b/rediscluster/connection.py @@ -216,6 +216,8 @@ def get_connection_by_slot(self, slot): """ Determine what server a specific slot belongs to and return a redis object that is connected """ + self._checkpid() + try: return self.get_connection_by_node(self.get_node_by_slot(slot)) except KeyError: @@ -225,6 +227,7 @@ def get_connection_by_node(self, node): """ get a connection by node """ + self._checkpid() self.nodes.set_node_name(node) try: From d454f8816a4b3b046c6efe77e9036a565b2f14d6 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 8 Sep 2015 20:57:38 +0200 Subject: [PATCH 18/50] Add note in CHANGES about improved thread safty --- CHANGES | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES b/CHANGES index 4f7429b7..d1bd831c 100644 --- a/CHANGES +++ b/CHANGES @@ -3,6 +3,7 @@ * Added READONLY mode support, scales reads using slave nodes. * Fix __repr__ for ClusterConnectionPool and ClusterReadOnlyConnectionPool * Add max_connections_per_node parameter to ClusterConnectionPool so that max_connections parameter is calculated per-node rather than across the whole cluster. + * Improve thread safty of get_connection_by_slot and get_connection_by_node methods (iandyh) * 1.0.0 * No change to anything just a bump to 1.0.0 because the lib is now considered stable/production ready. From e4c4028010c26ca1c18ef6f400d7e50e74b96f65 Mon Sep 17 00:00:00 2001 From: iandyh Date: Tue, 22 Sep 2015 14:22:20 +0900 Subject: [PATCH 19/50] reset the connection when there the connection is broken. Copy from redis-py --- rediscluster/client.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/rediscluster/client.py b/rediscluster/client.py index ce9b83ba..db521547 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -269,12 +269,23 @@ def _execute_command_on_nodes(self, nodes, *args, **kwargs): command = args[0] res = {} for node in nodes: - r = self.connection_pool.get_connection_by_node(node) + connection = self.connection_pool.get_connection_by_node(node) + + # copy from redis-py try: - r.send_command(*args) - res[node["name"]] = self.parse_response(r, command, **kwargs) + connection.send_command(*args) + res[node["name"]] = self.parse_response(connection, command, + **kwargs) + except (ConnectionError, TimeoutError) as e: + connection.disconnect() + if (not connection.retry_on_timeout and + isinstance(e, TimeoutError)): + raise + connection.send_command(*args) + res[node["name"]] = self.parse_response(connection, + command, **kwargs) finally: - self.connection_pool.release(r) + self.connection_pool.release(connection) return self._merge_result(command, res) From efae6520cd837a62f991d6741bf81b8c59d7b853 Mon Sep 17 00:00:00 2001 From: iandyh Date: Mon, 28 Sep 2015 17:57:09 +0900 Subject: [PATCH 20/50] fix connections in cluster_mgt --- rediscluster/cluster_mgt.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/rediscluster/cluster_mgt.py b/rediscluster/cluster_mgt.py index 4b3f47f2..cfd7ceb1 100644 --- a/rediscluster/cluster_mgt.py +++ b/rediscluster/cluster_mgt.py @@ -5,6 +5,8 @@ from .exceptions import RedisClusterException from .utils import clusterdown_wrapper, first_key, nslookup +from redis.exceptions import ConnectionError, TimeoutError + class RedisClusterMgt(object): @@ -33,6 +35,13 @@ def _execute_command_on_nodes(self, nodes, *args, **kwargs): try: c.send_command(*args) res[node["name"]] = c.read_response() + except (ConnectionError, TimeoutError) as e: + c.disconnect() + if (not c.retry_on_timeout and + isinstance(e, TimeoutError)): + raise + c.send_command(*args) + res[node["name"]] = c.read_response() finally: self.connection_pool.release(c) return first_key(command, res) From 96ec07a768a7b430f533826ae91a5af425a4fd40 Mon Sep 17 00:00:00 2001 From: iandyh Date: Wed, 30 Sep 2015 16:59:00 +0900 Subject: [PATCH 21/50] install coverage version greater than 3.7.1 and less than 4.0.0. Because in 4.0.0, coverage drops support for Python 3.2 --- dev-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-requirements.txt b/dev-requirements.txt index f9ae0415..b54e5386 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,6 +1,6 @@ -r requirements.txt -coverage >= 3.7.1 +coverage >= 3.7.1,< 4.0.0 -e git://github.com/pytest-dev/pytest.git@master#egg=pytest testfixtures >= 4.0.1 mock == 1.0.1 From d53dc85af83f820da48e344d06acb93d36b7a3d7 Mon Sep 17 00:00:00 2001 From: iandyh Date: Thu, 1 Oct 2015 14:25:25 +0900 Subject: [PATCH 22/50] update CHANGES --- CHANGES | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGES b/CHANGES index d1bd831c..31b56a82 100644 --- a/CHANGES +++ b/CHANGES @@ -1,9 +1,10 @@ * Next release - * Refactored exception handling and exception classes. + * Refactored exception handling and exception classes. * Added READONLY mode support, scales reads using slave nodes. * Fix __repr__ for ClusterConnectionPool and ClusterReadOnlyConnectionPool * Add max_connections_per_node parameter to ClusterConnectionPool so that max_connections parameter is calculated per-node rather than across the whole cluster. * Improve thread safty of get_connection_by_slot and get_connection_by_node methods (iandyh) + * Improved error handling when sending commands to all nodes, e.g. info. Now the connection takes retry_on_timeout as an option and retry once when there is a timeout. (iandyh) * 1.0.0 * No change to anything just a bump to 1.0.0 because the lib is now considered stable/production ready. @@ -19,9 +20,9 @@ * 0.2.0 * Moved pipeline code into new file. - * Code now uses a proper cluster connection pool class that handles + * Code now uses a proper cluster connection pool class that handles all nodes and connections similar to how redis-py do. - * Better support for pubsub. All clients will now talk to the same server because + * Better support for pubsub. All clients will now talk to the same server because pubsub commands do not work reliably if it talks to a random server in the cluster. * Better result callbacks and node routing support. No more ugly decorators. * Fix keyslot command when using non ascii characters. From 0ed81fbc3278e957d309faaf55d0470e01e07d72 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Wed, 30 Sep 2015 23:56:13 +0200 Subject: [PATCH 23/50] Now test python 3.5 compatibility. Now tests against nightly python build (3.6) with allowed failure. --- .travis.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 1bb1e619..08b66d51 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,7 +5,8 @@ python: - "3.2" - "3.3" - "3.4" - - "3.5-dev" + - "3.5" + - "nightly" services: - redis-server install: @@ -26,4 +27,4 @@ after_success: - coveralls matrix: allow_failures: - - python: "3.5-dev" + - python: "nightly" From 329181ac220257a2fd79d7244e2bba785ae626da Mon Sep 17 00:00:00 2001 From: orthographic-pedant Date: Mon, 5 Oct 2015 11:53:51 -0400 Subject: [PATCH 24/50] Fix typographical error(s) Changed carefull to careful in README. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 0d06af8c..48a9cb34 100644 --- a/README.md +++ b/README.md @@ -119,7 +119,7 @@ More detailed documentation can be found in `docs` folder. Both Redis cluster and redis-py-cluster is considered stable and production ready. -But this depends on what you are going to use clustering for. In the simple use cases with SET/GET and other single key functions there is not issues. If you require multi key functinoality or pipelines then you must be very carefull when developing because they work slightly different from the normal redis server. +But this depends on what you are going to use clustering for. In the simple use cases with SET/GET and other single key functions there is not issues. If you require multi key functinoality or pipelines then you must be very careful when developing because they work slightly different from the normal redis server. If you require advance features like pubsub or scripting, this lib and redis do not handle that kind of use-cases very well. You either need to develop a custom solution yourself or use a non clustered redis server for that. From 6a768db3c978d4f6e488199ab73ab314685c57e3 Mon Sep 17 00:00:00 2001 From: Ali-Akber Saifee Date: Tue, 6 Oct 2015 22:31:36 +0500 Subject: [PATCH 25/50] Enable SCRIPT commands LOAD/FLUSH/EXISTS/EVALSHA --- rediscluster/client.py | 33 ++++++++++++++++++++------------- tests/test_cluster_obj.py | 3 +-- tests/test_scripting.py | 5 ----- 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/rediscluster/client.py b/rediscluster/client.py index db521547..b78e7b16 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -21,7 +21,6 @@ first_key, clusterdown_wrapper, ) - # 3rd party imports from redis import StrictRedis from redis.client import list_or_args @@ -40,18 +39,21 @@ class StrictRedisCluster(StrictRedis): string_keys_to_dict([ "CLIENT SETNAME", "SENTINEL GET-MASTER-ADDR-BY-NAME", 'SENTINEL MASTER', 'SENTINEL MASTERS', 'SENTINEL MONITOR', 'SENTINEL REMOVE', 'SENTINEL SENTINELS', 'SENTINEL SET', - 'SENTINEL SLAVES', 'SHUTDOWN', 'SLAVEOF', 'EVALSHA', 'SCRIPT EXISTS', 'SCRIPT KILL', - 'SCRIPT LOAD', 'MOVE', 'BITOP', + 'SENTINEL SLAVES', 'SHUTDOWN', 'SLAVEOF', 'SCRIPT KILL', + 'MOVE', 'BITOP', ], blocked_command), string_keys_to_dict([ "ECHO", "CONFIG GET", "CONFIG SET", "SLOWLOG GET", "CLIENT KILL", "INFO", "BGREWRITEAOF", "BGSAVE", "CLIENT LIST", "CLIENT GETNAME", "CONFIG RESETSTAT", "CONFIG REWRITE", "DBSIZE", "LASTSAVE", "PING", "SAVE", "SLOWLOG LEN", "SLOWLOG RESET", - "TIME", "SCRIPT FLUSH", "SCAN", + "TIME", "SCAN", ], lambda self, command: self.connection_pool.nodes.all_nodes()), string_keys_to_dict([ "FLUSHALL", "FLUSHDB", ], lambda self, command: self.connection_pool.nodes.all_masters()), + string_keys_to_dict([ + "SCRIPT LOAD", "SCRIPT FLUSH", "SCRIPT EXISTS", + ], lambda self, command: self.connection_pool.nodes.all_masters()), string_keys_to_dict([ "KEYS", ], lambda self, command: self.connection_pool.nodes.all_nodes()), @@ -68,11 +70,20 @@ class StrictRedisCluster(StrictRedis): "ECHO", "CONFIG GET", "CONFIG SET", "SLOWLOG GET", "CLIENT KILL", "INFO", "BGREWRITEAOF", "BGSAVE", "CLIENT LIST", "CLIENT GETNAME", "CONFIG RESETSTAT", "CONFIG REWRITE", "DBSIZE", "LASTSAVE", "PING", "SAVE", "SLOWLOG LEN", "SLOWLOG RESET", - "TIME", "SCRIPT FLUSH", "SCAN", + "TIME", "SCAN", ], lambda command, res: res), string_keys_to_dict([ "FLUSHALL", "FLUSHDB", ], lambda command, res: res), + string_keys_to_dict([ + "SCRIPT LOAD", + ], lambda command, res: list(res.values()).pop()), + string_keys_to_dict([ + "SCRIPT EXISTS", + ], lambda command, res: [all(k) for k in zip(*res.values())]), + string_keys_to_dict([ + "SCRIPT FLUSH", + ], lambda command, res: all(res.values())), string_keys_to_dict([ "KEYS", ], merge_result), @@ -171,12 +182,14 @@ def _determine_slot(self, *args): raise RedisClusterException("No way to dispatch this command to Redis Cluster. Missing key.") command = args[0] - if command == 'EVAL': + if command in ['EVAL', 'EVALSHA']: numkeys = args[2] keys = args[3: 3 + numkeys] slots = set([self.connection_pool.nodes.keyslot(key) for key in keys]) if len(slots) != 1: - raise RedisClusterException("EVAL - all keys must map to the same key slot") + raise RedisClusterException( + "%s - all keys must map to the same key slot" % command + ) return slots.pop() key = args[1] @@ -767,12 +780,6 @@ def _random_id(self, size=16, chars=string.ascii_uppercase + string.digits): """ return ''.join(random.choice(chars) for _ in range(size)) - def register_script(self, script): - """ - Scripts is not yet supported by rediscluster. - """ - raise RedisClusterException("Method register_script is not possible to use in a redis cluster") - class RedisCluster(StrictRedisCluster): """ diff --git a/tests/test_cluster_obj.py b/tests/test_cluster_obj.py index 09a2a8e9..cad53693 100644 --- a/tests/test_cluster_obj.py +++ b/tests/test_cluster_obj.py @@ -72,8 +72,7 @@ def test_blocked_commands(r): blocked_commands = [ "CLIENT SETNAME", "SENTINEL GET-MASTER-ADDR-BY-NAME", 'SENTINEL MASTER', 'SENTINEL MASTERS', 'SENTINEL MONITOR', 'SENTINEL REMOVE', 'SENTINEL SENTINELS', 'SENTINEL SET', - 'SENTINEL SLAVES', 'SHUTDOWN', 'SLAVEOF', 'EVALSHA', 'SCRIPT EXISTS', 'SCRIPT KILL', - 'SCRIPT LOAD', 'MOVE', 'BITOP', + 'SENTINEL SLAVES', 'SHUTDOWN', 'SLAVEOF', 'SCRIPT KILL', 'MOVE', 'BITOP', ] for command in blocked_commands: diff --git a/tests/test_scripting.py b/tests/test_scripting.py index 91a836cf..84b9eb1b 100644 --- a/tests/test_scripting.py +++ b/tests/test_scripting.py @@ -18,7 +18,6 @@ return value * ARGV[1]""" -@pytest.mark.xfail(reason="scripting is not yet supported") class TestScripting(object): @pytest.fixture(autouse=True) def reset_scripts(self, r): @@ -59,14 +58,12 @@ def test_eval_crossslot(self, r): with pytest.raises(RedisClusterException): r.eval(script, 2, 'A{foo}', 'B{bar}') - @pytest.mark.xfail(reason="Not Yet Implemented") def test_evalsha(self, r): r.set('a', 2) sha = r.script_load(multiply_script) # 2 * 3 == 6 assert r.evalsha(sha, 1, 'a', 3) == 6 - @pytest.mark.xfail(reason="Not Yet Implemented") def test_evalsha_script_not_loaded(self, r): r.set('a', 2) sha = r.script_load(multiply_script) @@ -75,7 +72,6 @@ def test_evalsha_script_not_loaded(self, r): with pytest.raises(exceptions.NoScriptError): r.evalsha(sha, 1, 'a', 3) - @pytest.mark.xfail(reason="Not Yet Implemented") def test_script_loading(self, r): # get the sha, then clear the cache sha = r.script_load(multiply_script) @@ -84,7 +80,6 @@ def test_script_loading(self, r): r.script_load(multiply_script) assert r.script_exists(sha) == [True] - @pytest.mark.xfail(reason="Not Yet Implemented") def test_script_object(self, r): r.set('a', 2) multiply = r.register_script(multiply_script) From 317f981b28bc5dc413ac2813466616e355ee1ce9 Mon Sep 17 00:00:00 2001 From: Ali-Akber Saifee Date: Thu, 8 Oct 2015 13:56:45 +0500 Subject: [PATCH 26/50] Update Authors & CHANGES documents --- CHANGES | 1 + docs/Authors | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index 31b56a82..dac08db2 100644 --- a/CHANGES +++ b/CHANGES @@ -5,6 +5,7 @@ * Add max_connections_per_node parameter to ClusterConnectionPool so that max_connections parameter is calculated per-node rather than across the whole cluster. * Improve thread safty of get_connection_by_slot and get_connection_by_node methods (iandyh) * Improved error handling when sending commands to all nodes, e.g. info. Now the connection takes retry_on_timeout as an option and retry once when there is a timeout. (iandyh) + * Added support for SCRIPT LOAD, SCRIPT FLUSH, SCRIPT EXISTS and EVALSHA commands. (alisaifee) * 1.0.0 * No change to anything just a bump to 1.0.0 because the lib is now considered stable/production ready. diff --git a/docs/Authors b/docs/Authors index 3ccb0bb4..c30dce7e 100644 --- a/docs/Authors +++ b/docs/Authors @@ -20,3 +20,4 @@ Authors who contributed code or testing: - iandyh - https://github.com/iandyh - mumumu - https://github.com/mumumu - awestendorf - https://github.com/awestendorf + - Ali-Akber Saifee - https://github.com/alisaifee \ No newline at end of file From 1c41e184b07e5d781b5de440c04963b93847c5d5 Mon Sep 17 00:00:00 2001 From: Ali-Akber Saifee Date: Thu, 8 Oct 2015 15:40:33 +0500 Subject: [PATCH 27/50] Add documentation for scripting Updated the command documentation, and briefly described the limitations of the current scripting support. --- docs/Commands.md | 17 ++++++++++------- docs/Scripting.md | 27 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 7 deletions(-) create mode 100644 docs/Scripting.md diff --git a/docs/Commands.md b/docs/Commands.md index 4f848fb3..59253335 100644 --- a/docs/Commands.md +++ b/docs/Commands.md @@ -26,7 +26,6 @@ The following commands will send the same request to all nodes in the cluster. R - lastsave - ping - save - - script_flush - slowlog_get - slowlog_len - slowlog_reset @@ -42,13 +41,21 @@ The following commands will only be send to the master nodes in the cluster. Res - flushdb - scan + This command will sent to a random node in the cluster. - publish -This command will be sent to the server that matches the first key. +The following commands will be sent to the server that matches the first key. - eval + - evalsha + +This following commands will be sent to the master nodes in the cluster. + +- script load - the result is the hash of loaded script +- script flush - the result is `True` if the command succeeds on all master nodes, else `False` +- script exists - the result is an array of booleans. An entry is `True` only if the script exists on all the master nodes. The following commands will be sent to the sever that matches the specefied key. @@ -70,13 +77,9 @@ Either because they do not work, there is no working implementation or it is not - bitop - Currently to hard to implement a solution in python space - client_setname - Not yet implemented - - evalsha - Lua scripting is not yet implemented - move - It is not possible to move a key from one db to another in cluster mode - - register_script - Lua scripting is not yet implemented - restore - - script_exists - Lua scripting is not yet implemented - - script_kill - Lua scripting is not yet implemented - - script_load - Lua scripting is not yet implemented + - script_kill - Not yet implemented - sentinel - sentinel_get_master_addr_by_name - sentinel_master diff --git a/docs/Scripting.md b/docs/Scripting.md new file mode 100644 index 00000000..683f1e6f --- /dev/null +++ b/docs/Scripting.md @@ -0,0 +1,27 @@ +# Scripting support + +Scripting support is limited to scripts that operate on keys in the same key slot. +If a script is executed via `evalsha`, `eval` or by calling the callable returned by +`register_script` and the keys passed as arguments do not map to the same key slot, +a `RedisClusterException` will be thrown. + +It is however, possible to query a key within the script, that is not passed +as an argument of `eval`, `evalsha`. In this scenarios it is not possible to detect +the error early and redis itself will raise an error which will be percolated +to the user. For example: + +```python + cluster = RedisCluster('localhost', 7000) + script = """ + return redis.call('GET', KEYS[1]) * redis.call('GET', ARGV[1]) + """ + # this will succeed + cluster.eval(script, 1, "A{Foo}", "A{Foo}") + # this will fail as "A{Foo}" and "A{Bar}" are on different key slots. + cluster.eval(script, 1, "A{Foo}", "A{Bar}") +``` + +## Unsupported operations + +- The `SCRIPT KILL` command is not yet implemented. +- Scripting in the context of a pipeline is not yet implemented. From 06e5e53bb2b13132d81802892a73f9e6042c1bf5 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sat, 3 Oct 2015 20:22:19 +0200 Subject: [PATCH 28/50] WIP --- rediscluster/nodemanager.py | 54 ++++++++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 15 deletions(-) diff --git a/rediscluster/nodemanager.py b/rediscluster/nodemanager.py index b2deb10b..e920b5c7 100644 --- a/rediscluster/nodemanager.py +++ b/rediscluster/nodemanager.py @@ -75,13 +75,32 @@ def initialize(self): Maybe it should stop to try after it have correctly covered all slots or when one node is reached and it could execute CLUSTER SLOTS command. """ - # Reset variables - self.flush_nodes_cache() - self.flush_slots_cache() + nodes_cache = {} + + def _set_node(host, port, server_type=None): + """ + TODO: Temporary location for this method + + Update data for a node. + """ + node_name = "{0}:{1}".format(host, port) + nodes_cache.setdefault(node_name, {}) + nodes_cache[node_name]['host'] = host + nodes_cache[node_name]['port'] = port + nodes_cache[node_name]['name'] = node_name + + if server_type: + nodes_cache[node_name]['server_type'] = server_type + + return nodes_cache[node_name] + + tmp_slots = {} + all_slots_covered = False disagreements = [] startup_nodes_reachable = False - for node in self.startup_nodes: + + for node in self.orig_startup_nodes: try: r = self.get_redis_link(host=node["host"], port=node["port"], decode_responses=True) cluster_slots = r.execute_command("cluster", "slots") @@ -106,19 +125,19 @@ def initialize(self): master_node[0] = node['host'] master_node[1] = int(master_node[1]) - node = self.set_node(master_node[0], master_node[1], server_type='master') + node = _set_node(master_node[0], master_node[1], server_type='master') for i in range(int(slot[0]), int(slot[1]) + 1): - if i not in self.slots: - self.slots[i] = [node] + if i not in tmp_slots: + tmp_slots[i] = [node] slave_nodes = [slot[j] for j in range(3, len(slot))] for slave_node in slave_nodes: - target_slave_node = self.set_node(slave_node[0], slave_node[1], server_type='slave') - self.slots[i].append(target_slave_node) + target_slave_node = _set_node(slave_node[0], slave_node[1], server_type='slave') + tmp_slots[i].append(target_slave_node) else: # Validate that 2 nodes want to use the same slot cache setup - if self.slots[i][0]['name'] != node['name']: - disagreements.append("{} vs {} on slot: {}".format(self.slots[i][0]['name'], node['name'], i)) + if tmp_slots[i][0]['name'] != node['name']: + disagreements.append("{} vs {} on slot: {}".format(tmp_slots[i][0]['name'], node['name'], i)) if len(disagreements) > 5: raise RedisClusterException("startup_nodes could not agree on a valid slots cache. %s" % ", ".join(disagreements)) @@ -127,21 +146,26 @@ def initialize(self): # Validate if all slots are covered or if we should try next startup node for i in range(0, self.RedisClusterHashSlots): - if i not in self.slots: + if i not in tmp_slots: all_slots_covered = False if all_slots_covered: # All slots are covered and application can continue to execute # Parse and determine what node will be pubsub node - self.determine_pubsub_node() - return + break if not startup_nodes_reachable: raise RedisClusterException("Redis Cluster cannot be connected. Please provide at least one reachable node.") if not all_slots_covered: raise RedisClusterException("All slots are not covered after query all startup_nodes. {} of {} covered...".format( - len(self.slots), self.RedisClusterHashSlots)) + len(tmp_slots), self.RedisClusterHashSlots)) + + # Set the tmp variables to the real variables + self.slots = tmp_slots + self.nodes = nodes_cache + + self.determine_pubsub_node() def determine_pubsub_node(self): """ From 260ef6be64c6d2a42a0c0419b0eef6572308567a Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sun, 4 Oct 2015 01:28:48 +0200 Subject: [PATCH 29/50] Simplify things when creating nodes --- rediscluster/nodemanager.py | 53 +++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 29 deletions(-) diff --git a/rediscluster/nodemanager.py b/rediscluster/nodemanager.py index e920b5c7..2947503b 100644 --- a/rediscluster/nodemanager.py +++ b/rediscluster/nodemanager.py @@ -76,24 +76,6 @@ def initialize(self): and it could execute CLUSTER SLOTS command. """ nodes_cache = {} - - def _set_node(host, port, server_type=None): - """ - TODO: Temporary location for this method - - Update data for a node. - """ - node_name = "{0}:{1}".format(host, port) - nodes_cache.setdefault(node_name, {}) - nodes_cache[node_name]['host'] = host - nodes_cache[node_name]['port'] = port - nodes_cache[node_name]['name'] = node_name - - if server_type: - nodes_cache[node_name]['server_type'] = server_type - - return nodes_cache[node_name] - tmp_slots = {} all_slots_covered = False @@ -125,14 +107,17 @@ def _set_node(host, port, server_type=None): master_node[0] = node['host'] master_node[1] = int(master_node[1]) - node = _set_node(master_node[0], master_node[1], server_type='master') + node, node_name = self.make_node_obj(master_node[0], master_node[1], 'master') + nodes_cache[node_name] = node for i in range(int(slot[0]), int(slot[1]) + 1): if i not in tmp_slots: tmp_slots[i] = [node] slave_nodes = [slot[j] for j in range(3, len(slot))] + for slave_node in slave_nodes: - target_slave_node = _set_node(slave_node[0], slave_node[1], server_type='slave') + target_slave_node, slave_node_name = self.make_node_obj(slave_node[0], slave_node[1], 'slave') + nodes_cache[slave_node_name] = target_slave_node tmp_slots[i].append(target_slave_node) else: # Validate that 2 nodes want to use the same slot cache setup @@ -193,20 +178,30 @@ def set_node_name(self, n): if "name" not in n: n["name"] = "{0}:{1}".format(n["host"], n["port"]) - def set_node(self, host, port, server_type=None): + def make_node_obj(self, host, port, server_type): """ - Update data for a node. + Create a node datastructure. + + Returns the node datastructure and the node name """ node_name = "{0}:{1}".format(host, port) - self.nodes.setdefault(node_name, {}) - self.nodes[node_name]['host'] = host - self.nodes[node_name]['port'] = port - self.nodes[node_name]['name'] = node_name + node = { + 'host': host, + 'port': port, + 'name': node_name, + 'server_type': server_type + } - if server_type: - self.nodes[node_name]['server_type'] = server_type + return (node, node_name) + + def set_node(self, host, port, server_type=None): + """ + Update data for a node. + """ + node, node_name = self.make_node_obj(host, port, server_type) + self.nodes[node_name] = node - return self.nodes[node_name] + return node def populate_startup_nodes(self): """ From 8a671f6a26c440a199b1d36da8f633b9862c1aa4 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sun, 4 Oct 2015 11:28:14 +0200 Subject: [PATCH 30/50] Cleanup unused methods --- rediscluster/nodemanager.py | 14 -------------- tests/test_node_manager.py | 16 ---------------- 2 files changed, 30 deletions(-) diff --git a/rediscluster/nodemanager.py b/rediscluster/nodemanager.py index 2947503b..033b4f96 100644 --- a/rediscluster/nodemanager.py +++ b/rediscluster/nodemanager.py @@ -221,18 +221,4 @@ def reset(self): """ Drop all node data and start over from startup_nodes """ - self.flush_nodes_cache() - self.flush_slots_cache() self.initialize() - - def flush_slots_cache(self): - """ - Reset slots cache back to empty dict - """ - self.slots = {} - - def flush_nodes_cache(self): - """ - Reset nodes cache back to empty dict - """ - self.nodes = {} diff --git a/tests/test_node_manager.py b/tests/test_node_manager.py index d98da004..c7c69eae 100644 --- a/tests/test_node_manager.py +++ b/tests/test_node_manager.py @@ -118,22 +118,6 @@ def test_wrong_startup_nodes_type(): NodeManager({}) -def test_flush_slots_nodes_cache(): - """ - Slots cache should already be populated. - """ - n = NodeManager([{"host": "127.0.0.1", "port": 7000}]) - n.initialize() - assert len(n.slots) == NodeManager.RedisClusterHashSlots - assert len(n.nodes) == 6 - - n.flush_slots_cache() - n.flush_nodes_cache() - - assert len(n.slots) == 0 - assert len(n.nodes) == 0 - - def test_init_slots_cache_slots_collision(): """ Test that if 2 nodes do not agree on the same slots setup it should raise an error. From c1368450faf267b3adaa81d12857fbdeefb67585 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sun, 4 Oct 2015 12:07:12 +0200 Subject: [PATCH 31/50] Fix some tests that was broken after the test server has been resharded a few times --- tests/test_cluster_connection_pool.py | 22 +++++++++++++----- tests/test_cluster_obj.py | 32 ++++++++++++++++----------- tests/test_node_manager.py | 6 +---- 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/tests/test_cluster_connection_pool.py b/tests/test_cluster_connection_pool.py index 7e569612..e17dfa4a 100644 --- a/tests/test_cluster_connection_pool.py +++ b/tests/test_cluster_connection_pool.py @@ -17,7 +17,7 @@ # 3rd party imports import pytest import redis -from mock import Mock +from mock import patch, Mock from redis.connection import ssl_available from redis._compat import unicode @@ -110,8 +110,14 @@ def test_get_connection_by_key(self): """ pool = self.get_pool(connection_kwargs={}) - connection = pool.get_connection_by_key("foo") - assert connection.port == 7002 + # Patch the call that is made inside the method to allow control of the returned connection object + with patch.object(ClusterConnectionPool, 'get_connection_by_slot', autospec=True) as pool_mock: + def side_effect(self, *args, **kwargs): + return DummyConnection(port=1337) + pool_mock.side_effect = side_effect + + connection = pool.get_connection_by_key("foo") + assert connection.port == 1337 with pytest.raises(RedisClusterException) as ex: pool.get_connection_by_key(None) @@ -123,8 +129,14 @@ def test_get_connection_by_slot(self): """ pool = self.get_pool(connection_kwargs={}) - connection = pool.get_connection_by_slot(12182) - assert connection.port == 7002 + # Patch the call that is made inside the method to allow control of the returned connection object + with patch.object(ClusterConnectionPool, 'get_connection_by_node', autospec=True) as pool_mock: + def side_effect(self, *args, **kwargs): + return DummyConnection(port=1337) + pool_mock.side_effect = side_effect + + connection = pool.get_connection_by_slot(12182) + assert connection.port == 1337 m = Mock() pool.get_random_connection = m diff --git a/tests/test_cluster_obj.py b/tests/test_cluster_obj.py index cad53693..46fef4e5 100644 --- a/tests/test_cluster_obj.py +++ b/tests/test_cluster_obj.py @@ -182,19 +182,25 @@ def test_refresh_table_asap(): with patch.object(NodeManager, 'initialize') as mock_initialize: mock_initialize.return_value = None - r = StrictRedisCluster(host="127.0.0.1", port=7000) - r.connection_pool.nodes.slots[12182] = [{ - "host": "127.0.0.1", - "port": 7002, - "name": "127.0.0.1:7002", - "server_type": "master", - }] - r.refresh_table_asap = True - - i = len(mock_initialize.mock_calls) - r.execute_command("SET", "foo", "bar") - assert len(mock_initialize.mock_calls) - i == 1 - assert r.refresh_table_asap is False + # Patch parse_response to avoid issues when the cluster sometimes return MOVED + with patch.object(StrictRedisCluster, 'parse_response') as mock_parse_response: + def side_effect(self, *args, **kwargs): + return None + mock_parse_response.side_effect = side_effect + + r = StrictRedisCluster(host="127.0.0.1", port=7000) + r.connection_pool.nodes.slots[12182] = [{ + "host": "127.0.0.1", + "port": 7002, + "name": "127.0.0.1:7002", + "server_type": "master", + }] + r.refresh_table_asap = True + + i = len(mock_initialize.mock_calls) + r.execute_command("SET", "foo", "bar") + assert len(mock_initialize.mock_calls) - i == 1 + assert r.refresh_table_asap is False def test_ask_redirection(): diff --git a/tests/test_node_manager.py b/tests/test_node_manager.py index c7c69eae..99f789c1 100644 --- a/tests/test_node_manager.py +++ b/tests/test_node_manager.py @@ -270,12 +270,8 @@ def test_reset(): """ n = NodeManager(startup_nodes=[{}]) n.initialize = Mock() - n.slots = {"foo": "bar"} - n.nodes = ["foo", "bar"] n.reset() - - assert n.slots == {} - assert n.nodes == {} + assert n.initialize.call_count == 1 def test_cluster_one_instance(): From 8bd89f07f7689d88cd01c8d68c9d6d56f9e11b46 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 6 Oct 2015 10:11:47 +0200 Subject: [PATCH 32/50] Change so reinitialize is not runned on every MOVED error but instead every x number of moved error instead. The interval is configurable via the constructor. --- rediscluster/client.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/rediscluster/client.py b/rediscluster/client.py index b78e7b16..ba6f84c0 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -96,7 +96,7 @@ class StrictRedisCluster(StrictRedis): ) def __init__(self, host=None, port=None, startup_nodes=None, max_connections=32, init_slot_cache=True, - pipeline_use_threads=True, readonly_mode=False, **kwargs): + pipeline_use_threads=True, readonly_mode=False, reinitialize_steps=None, **kwargs): """ startup_nodes --> List of nodes that initial bootstrapping can be done from host --> Can be used to point to a startup node @@ -136,6 +136,8 @@ def __init__(self, host=None, port=None, startup_nodes=None, max_connections=32, self.result_callbacks = self.__class__.RESULT_CALLBACKS.copy() self.response_callbacks = self.__class__.RESPONSE_CALLBACKS.copy() self.pipeline_use_threads = pipeline_use_threads + self.reinitialize_counter = 0 + self.reinitialize_steps = reinitialize_steps or 25 def __repr__(self): servers = list(set(['{}:{}'.format(nativestr(info['host']), info['port']) for info in self.connection_pool.nodes.startup_nodes])) @@ -263,7 +265,14 @@ def execute_command(self, *args, **kwargs): self.refresh_table_asap = True raise e except MovedError as e: - self.refresh_table_asap = True + # Reinitialize on ever x number of MovedError. + # This counter will increase faster when the same client object + # is shared between multiple threads. To reduce the frequency you + # can set the variable 'reinitialize_steps' in the constructor. + self.reinitialize_counter += 1 + if self.reinitialize_counter % self.reinitialize_steps == 0: + self.refresh_table_asap = True + node = self.connection_pool.nodes.set_node(e.host, e.port, server_type='master') self.connection_pool.nodes.slots[e.slot_id][0] = node redirect_addr = "%s:%s" % (e.host, e.port) From 62917150279788c69c54c0d485b04c7ecd24ef00 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 6 Oct 2015 10:22:08 +0200 Subject: [PATCH 33/50] Add line in changes and Upgrading.md --- CHANGES | 2 ++ docs/Upgrading.md | 3 +++ 2 files changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index dac08db2..e7598d3e 100644 --- a/CHANGES +++ b/CHANGES @@ -6,6 +6,8 @@ * Improve thread safty of get_connection_by_slot and get_connection_by_node methods (iandyh) * Improved error handling when sending commands to all nodes, e.g. info. Now the connection takes retry_on_timeout as an option and retry once when there is a timeout. (iandyh) * Added support for SCRIPT LOAD, SCRIPT FLUSH, SCRIPT EXISTS and EVALSHA commands. (alisaifee) + * Improve thread safty to avoid exceptions when running one client object inside multiple threads and doing resharding of the + cluster at the same time. * 1.0.0 * No change to anything just a bump to 1.0.0 because the lib is now considered stable/production ready. diff --git a/docs/Upgrading.md b/docs/Upgrading.md index 7401cd37..fcd3494c 100644 --- a/docs/Upgrading.md +++ b/docs/Upgrading.md @@ -21,6 +21,9 @@ Added new `ClusterCrossSlotError` exception class. Added optional `max_connections_per_node` parameter to `ClusterConnectionPool` which changes behavior of `max_connections` so that it applies per-node rather than across the whole cluster. The new feature is opt-in, and the existing default behavior is unchanged. Users are recommended to opt-in as the feature fixes two important problems. First is that some nodes could be starved for connections after max_connections is used up by connecting to other nodes. Second is that the asymmetric number of connections across nodes makes it challenging to configure file descriptor and redis max client settings. +Reinitialize on `MOVED` errors will not run on every error but instead on every +25 error to avoid excessive cluster reinitialize when used in multiple threads and resharding at the same time. If you want to go back to the old behaviour with reinitialize on every error you should pass in `reinitialize_steps=1` to the client constructor. If you want to increase or decrease the intervall of this new behaviour you should set `reinitialize_steps` in the client constructor to a value that you want. + ## 0.2.0 --> 0.3.0 From 15363a1ba0d79c71af4b358ededc511a30b06226 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 6 Oct 2015 22:02:32 +0200 Subject: [PATCH 34/50] Fix broken pipelines while running in threads --- rediscluster/pipeline.py | 1 - 1 file changed, 1 deletion(-) diff --git a/rediscluster/pipeline.py b/rediscluster/pipeline.py index d9ac7fb0..d1e6e4ff 100644 --- a/rediscluster/pipeline.py +++ b/rediscluster/pipeline.py @@ -207,7 +207,6 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T raise v if isinstance(v, MovedError): - self.refresh_table_asap = True node = self.connection_pool.nodes.set_node(v.host, v.port, server_type='master') self.connection_pool.nodes.slots[v.slot_id][0] = node attempt.append(i) From 26f365767c655585ac6de7c274f1453606d752f8 Mon Sep 17 00:00:00 2001 From: John Loehrer Date: Wed, 7 Oct 2015 08:05:41 -0700 Subject: [PATCH 35/50] only release connection back to pool after thread join --- rediscluster/pipeline.py | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/rediscluster/pipeline.py b/rediscluster/pipeline.py index d1e6e4ff..4644f03f 100644 --- a/rediscluster/pipeline.py +++ b/rediscluster/pipeline.py @@ -162,9 +162,13 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T for node_name in node_commands: node = nodes[node_name] cmds = [node_commands[node_name][i] for i in sorted(node_commands[node_name].keys())] - with by_node_context(self.connection_pool, node) as connection: - workers[node_name] = Worker(self._execute_pipeline, connection, cmds, False) - workers[node_name].start() + + workers[node_name] = ThreadedPipelineExecute( + execute=self._execute_pipeline, + pool=self.connection_pool, + node=node, + cmds=cmds) + workers[node_name].start() for node_name, worker in workers.items(): worker.join() @@ -353,23 +357,23 @@ def inner(*args, **kwargs): StrictClusterPipeline.time = block_pipeline_command(StrictRedis.time) -class Worker(threading.Thread): +class ThreadedPipelineExecute(threading.Thread): """ - A simple thread wrapper class to perform an arbitrary function and store the result. - Used to parallelize network i/o when issuing many pipelined commands to multiple nodes in the cluster. - Later on we may need to convert this to a thread pool, although if you use gevent the current implementation - is not too bad. + Threaded pipeline execution. + release the connection back into the pool after executing. """ - def __init__(self, func, *args, **kwargs): + def __init__(self, execute, pool, node, cmds): threading.Thread.__init__(self) - self.func = func - self.args = args - self.kwargs = kwargs + self.execute = execute + self.pool = pool + self.node = node + self.cmds = cmds self.exception = None self.value = None def run(self): try: - self.value = self.func(*self.args, **self.kwargs) + with by_node_context(self.pool, self.node) as connection: + self.value = self.execute(connection, self.cmds, False) except Exception as e: self.exception = e From 46092a93742aba7c4ad914e153c04f660098e8af Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sat, 10 Oct 2015 19:16:10 +0200 Subject: [PATCH 36/50] Fix broken tests by use the same reinitialize steps logic as in normal client --- rediscluster/client.py | 3 ++- rediscluster/pipeline.py | 10 +++++++++- tests/conftest.py | 2 +- tests/test_cluster_obj.py | 15 +++++++-------- tests/test_pipeline.py | 4 ++-- 5 files changed, 21 insertions(+), 13 deletions(-) diff --git a/rediscluster/client.py b/rediscluster/client.py index ba6f84c0..5646ac2d 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -167,7 +167,8 @@ def pipeline(self, transaction=None, shard_hint=None, use_threads=None): nodes_callbacks=self.nodes_callbacks, result_callbacks=self.result_callbacks, response_callbacks=self.response_callbacks, - use_threads=self.pipeline_use_threads if use_threads is None else use_threads + use_threads=self.pipeline_use_threads if use_threads is None else use_threads, + reinitialize_steps=self.reinitialize_steps, ) def transaction(self, func, *watches, **kwargs): diff --git a/rediscluster/pipeline.py b/rediscluster/pipeline.py index 4644f03f..bc14a8f6 100644 --- a/rediscluster/pipeline.py +++ b/rediscluster/pipeline.py @@ -26,7 +26,7 @@ class StrictClusterPipeline(StrictRedisCluster): def __init__(self, connection_pool, nodes_callbacks=None, result_callbacks=None, response_callbacks=None, startup_nodes=None, refresh_table_asap=False, - use_threads=True): + use_threads=True, reinitialize_steps=None): self.connection_pool = connection_pool self.startup_nodes = startup_nodes if startup_nodes else [] self.refresh_table_asap = refresh_table_asap @@ -37,6 +37,9 @@ def __init__(self, connection_pool, nodes_callbacks=None, result_callbacks=None, self.response_callbacks = response_callbacks self.use_threads = use_threads + self.reinitialize_counter = 0 + self.reinitialize_steps = reinitialize_steps or 25 + def __repr__(self): return "{}".format(type(self).__name__) @@ -211,6 +214,11 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T raise v if isinstance(v, MovedError): + # Do not perform full cluster refresh on every MOVED error + self.reinitialize_counter += 1 + if self.reinitialize_counter % self.reinitialize_steps == 0: + self.refresh_table_asap = True + node = self.connection_pool.nodes.set_node(v.host, v.port, server_type='master') self.connection_pool.nodes.slots[v.slot_id][0] = node attempt.append(i) diff --git a/tests/conftest.py b/tests/conftest.py index 50dbd9fc..4f7ce05e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -124,7 +124,7 @@ def sr(request, *args, **kwargs): """ Returns a instance of StrictRedisCluster """ - return _init_client(request, cls=StrictRedisCluster, **kwargs) + return _init_client(request, reinitialize_steps=1, cls=StrictRedisCluster, **kwargs) @pytest.fixture() diff --git a/tests/test_cluster_obj.py b/tests/test_cluster_obj.py index 46fef4e5..82a06ba6 100644 --- a/tests/test_cluster_obj.py +++ b/tests/test_cluster_obj.py @@ -3,6 +3,7 @@ # python std lib from __future__ import with_statement import re +import time # rediscluster imports from rediscluster import StrictRedisCluster @@ -319,10 +320,10 @@ def ok_response(connection, command_name, **options): def assert_moved_redirection_on_slave(sr, connection_pool_cls, cluster_obj): - + """ + """ # we assume this key is set on 127.0.0.1:7000(7003) sr.set('foo16706', 'foo') - import time time.sleep(1) with patch.object(connection_pool_cls, 'get_node_by_slot') as return_slave_mock: @@ -334,10 +335,8 @@ def assert_moved_redirection_on_slave(sr, connection_pool_cls, cluster_obj): } master_value = {'host': '127.0.0.1', 'name': '127.0.0.1:7000', 'port': 7000, 'server_type': 'master'} - with patch.object( - ClusterConnectionPool, - 'get_master_node_by_slot', - return_value=master_value) as return_master_mock: + with patch.object(ClusterConnectionPool, 'get_master_node_by_slot') as return_master_mock: + return_master_mock.return_value = master_value assert cluster_obj.get('foo16706') == b('foo') assert return_master_mock.call_count == 1 @@ -350,7 +349,7 @@ def test_moved_redirection_on_slave_with_default_client(sr): assert_moved_redirection_on_slave( sr, ClusterConnectionPool, - StrictRedisCluster(host="127.0.0.1", port=7000) + StrictRedisCluster(host="127.0.0.1", port=7000, reinitialize_steps=1) ) @@ -361,7 +360,7 @@ def test_moved_redirection_on_slave_with_readonly_mode_client(sr): assert_moved_redirection_on_slave( sr, ClusterReadOnlyConnectionPool, - StrictRedisCluster(host="127.0.0.1", port=7000, readonly_mode=True) + StrictRedisCluster(host="127.0.0.1", port=7000, readonly_mode=True, reinitialize_steps=1) ) diff --git a/tests/test_pipeline.py b/tests/test_pipeline.py index 64ac5912..5f90c34f 100644 --- a/tests/test_pipeline.py +++ b/tests/test_pipeline.py @@ -517,7 +517,7 @@ def test_moved_redirection_on_slave_with_default(self): """ self.assert_moved_redirection_on_slave( ClusterConnectionPool, - StrictRedisCluster(host="127.0.0.1", port=7000) + StrictRedisCluster(host="127.0.0.1", port=7000, reinitialize_steps=1) ) def test_moved_redirection_on_slave_with_readonly_mode_client(self, sr): @@ -526,7 +526,7 @@ def test_moved_redirection_on_slave_with_readonly_mode_client(self, sr): """ self.assert_moved_redirection_on_slave( ClusterReadOnlyConnectionPool, - StrictRedisCluster(host="127.0.0.1", port=7000, readonly_mode=True) + StrictRedisCluster(host="127.0.0.1", port=7000, readonly_mode=True, reinitialize_steps=1) ) def test_access_correct_slave_with_readonly_mode_client(self, sr): From ac8be12052c2de86be107a03a3e98e5867721e9c Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sun, 18 Oct 2015 19:20:56 +0200 Subject: [PATCH 37/50] Reformat code and docstrings to make it alot easier to read and to not make it so compact. --- benchmarks/simple.py | 4 +- rediscluster/client.py | 101 ++++++++++++++++++-------- rediscluster/cluster_mgt.py | 28 +++++-- rediscluster/connection.py | 15 +++- rediscluster/nodemanager.py | 13 +++- rediscluster/pipeline.py | 28 +++++-- rediscluster/pubsub.py | 1 + rediscluster/utils.py | 8 ++ tests/test_cluster_connection_pool.py | 3 +- 9 files changed, 153 insertions(+), 48 deletions(-) diff --git a/benchmarks/simple.py b/benchmarks/simple.py index af30da85..c23495e6 100644 --- a/benchmarks/simple.py +++ b/benchmarks/simple.py @@ -65,7 +65,9 @@ def timeit_pipeline(rc, itterations=50000): p.execute() t1 = time.time() - t0 - print("{}k SET/GET operations inside pipelines took: {} seconds... {} operations per second".format((itterations / 1000) * 2, t1, (itterations / t1) * 2)) + print("{}k SET/GET operations inside pipelines took: {} seconds... {} operations per second".format( + (itterations / 1000) * 2, t1, (itterations / t1) * 2) + ) if __name__ == "__main__": diff --git a/rediscluster/client.py b/rediscluster/client.py index 5646ac2d..0334d42d 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -98,17 +98,24 @@ class StrictRedisCluster(StrictRedis): def __init__(self, host=None, port=None, startup_nodes=None, max_connections=32, init_slot_cache=True, pipeline_use_threads=True, readonly_mode=False, reinitialize_steps=None, **kwargs): """ - startup_nodes --> List of nodes that initial bootstrapping can be done from - host --> Can be used to point to a startup node - port --> Can be used to point to a startup node - max_connections --> Maximum number of connections that should be kept open at one time - pipeline_use_threads -> By default, use threads in pipeline if this flag is set to True - readonly_mode --> enable READONLY mode. You can read possibly stale data from slave. - **kwargs --> Extra arguments that will be sent into StrictRedis instance when created - (See Official redis-py doc for supported kwargs - [https://github.com/andymccurdy/redis-py/blob/master/redis/client.py]) - Some kwargs is not supported and will raise RedisClusterException - - db (Redis do not support database SELECT in cluster mode) + :startup_nodes: + List of nodes that initial bootstrapping can be done from + :host: + Can be used to point to a startup node + :port: + Can be used to point to a startup node + :max_connections: + Maximum number of connections that should be kept open at one time + :pipeline_use_threads: + By default, use threads in pipeline if this flag is set to True + :readonly_mode: + enable READONLY mode. You can read possibly stale data from slave. + :**kwargs: + Extra arguments that will be sent into StrictRedis instance when created + (See Official redis-py doc for supported kwargs + [https://github.com/andymccurdy/redis-py/blob/master/redis/client.py]) + Some kwargs is not supported and will raise RedisClusterException + - db (Redis do not support database SELECT in cluster mode) """ super(StrictRedisCluster, self).__init__(**kwargs) @@ -245,7 +252,8 @@ def execute_command(self, *args, **kwargs): r = self.connection_pool.get_random_connection() try_random_node = False else: - if self.refresh_table_asap: # MOVED + if self.refresh_table_asap: + # MOVED node = self.connection_pool.get_master_node_by_slot(slot) else: node = self.connection_pool.get_node_by_slot(slot) @@ -258,12 +266,14 @@ def execute_command(self, *args, **kwargs): raise except (ConnectionError, TimeoutError): try_random_node = True + if ttl < self.RedisClusterRequestTTL / 2: time.sleep(0.1) except ClusterDownError as e: self.connection_pool.disconnect() self.connection_pool.reset() self.refresh_table_asap = True + raise e except MovedError as e: # Reinitialize on ever x number of MovedError. @@ -289,24 +299,26 @@ def execute_command(self, *args, **kwargs): raise ClusterError('TTL exhausted.') def _execute_command_on_nodes(self, nodes, *args, **kwargs): + """ + """ command = args[0] res = {} + for node in nodes: connection = self.connection_pool.get_connection_by_node(node) # copy from redis-py try: connection.send_command(*args) - res[node["name"]] = self.parse_response(connection, command, - **kwargs) + res[node["name"]] = self.parse_response(connection, command, **kwargs) except (ConnectionError, TimeoutError) as e: connection.disconnect() - if (not connection.retry_on_timeout and - isinstance(e, TimeoutError)): + + if not connection.retry_on_timeout and isinstance(e, TimeoutError): raise + connection.send_command(*args) - res[node["name"]] = self.parse_response(connection, - command, **kwargs) + res[node["name"]] = self.parse_response(connection, command, **kwargs) finally: self.connection_pool.release(connection) @@ -330,6 +342,7 @@ def scan_iter(self, match=None, count=None): while cursor != 0: for _, node_data in self.scan(cursor=cursor, match=match, count=count).items(): cursor, data = node_data + for item in data: yield item @@ -359,8 +372,10 @@ def mset(self, *args, **kwargs): if len(args) != 1 or not isinstance(args[0], dict): raise RedisError('MSET requires **kwargs or a single dict arg') kwargs.update(args[0]) + for pair in iteritems(kwargs): self.set(pair[0], pair[1]) + return True def msetnx(self, *args, **kwargs): @@ -397,14 +412,19 @@ def rename(self, src, dst): raise ResponseError("source and destination objects are the same") data = self.dump(src) + if data is None: raise ResponseError("no such key") + ttl = self.pttl(src) + if ttl is None or ttl < 1: ttl = 0 + self.delete(dst) self.restore(dst, ttl, data) self.delete(src) + return True def delete(self, *names): @@ -418,8 +438,10 @@ def delete(self, *names): Operation is no longer atomic. """ count = 0 + for arg in names: count += self.execute_command('DEL', arg) + return count def renamenx(self, src, dst): @@ -433,8 +455,8 @@ def renamenx(self, src, dst): """ if not self.exists(dst): return self.rename(src, dst) - else: - return False + + return False #### # List commands @@ -475,29 +497,36 @@ def rpoplpush(self, src, dst): Operation is no longer atomic. """ value = self.rpop(src) + if value: self.lpush(dst, value) return value + return None def sort(self, name, start=None, num=None, by=None, get=None, desc=False, alpha=False, store=None, groups=None): """Sort and return the list, set or sorted set at ``name``. - ``start`` and ``num`` allow for paging through the sorted data + :start: and :num: + allow for paging through the sorted data - ``by`` allows using an external key to weight and sort the items. + :by: + allows using an external key to weight and sort the items. Use an "*" to indicate where in the key the item value is located - ``get`` allows for returning items from external keys rather than the + :get: + allows for returning items from external keys rather than the sorted data itself. Use an "*" to indicate where int he key the item value is located - ``desc`` allows for reversing the sort + :desc: + allows for reversing the sort - ``alpha`` allows for sorting lexicographically rather than numerically + :alpha: + allows for sorting lexicographically rather than numerically - ``store`` allows for storing the result of the sort into - the key ``store`` + :store: + allows for storing the result of the sort into the key `store` ClusterImpl: A full implementation of the server side sort mechanics because many of the @@ -633,8 +662,10 @@ def sdiff(self, keys, *args): """ k = list_or_args(keys, args) res = self.smembers(k[0]) + for arg in k[1:]: res = res - self.smembers(arg) + return res def sdiffstore(self, dest, keys, *args): @@ -648,6 +679,7 @@ def sdiffstore(self, dest, keys, *args): """ res = self.sdiff(keys, *args) self.delete(dest) + return self.sadd(dest, *res) def sinter(self, keys, *args): @@ -659,8 +691,10 @@ def sinter(self, keys, *args): """ k = list_or_args(keys, args) res = self.smembers(k[0]) + for arg in k[1:]: res = res & self.smembers(arg) + return res def sinterstore(self, dest, keys, *args): @@ -673,6 +707,7 @@ def sinterstore(self, dest, keys, *args): """ res = self.sinter(keys, *args) self.delete(dest) + if len(res) != 0: self.sadd(dest, *res) return len(res) @@ -705,8 +740,10 @@ def sunion(self, keys, *args): """ k = list_or_args(keys, args) res = self.smembers(k[0]) + for arg in k[1:]: res = res | self.smembers(arg) + return res def sunionstore(self, dest, keys, *args): @@ -721,6 +758,7 @@ def sunionstore(self, dest, keys, *args): """ res = self.sunion(keys, *args) self.delete(dest) + return self.sadd(dest, *res) def pfmerge(self, dest, *sources): @@ -749,6 +787,7 @@ def pfmerge(self, dest, *sources): # Special handling of dest variable if it allready exists, then it shold be included in the HLL merge # dest can exists anywhere in the cluster. dest_data = self.get(dest) + if dest_data: all_hll_objects.append(dest_data) @@ -768,6 +807,7 @@ def pfmerge(self, dest, *sources): # Cleanup tmp variables self.delete(tmp_dest) + for k in all_k: self.delete(k) @@ -838,6 +878,7 @@ def setex(self, name, value, time): """ if isinstance(time, datetime.timedelta): time = time.seconds + time.days * 24 * 3600 + return self.execute_command('SETEX', name, time, value) def lrem(self, name, value, num=0): @@ -868,14 +909,16 @@ def zadd(self, name, *args, **kwargs): redis.zadd('my-key', 'name1', 1.1, 'name2', 2.2, name3=3.3, name4=4.4) """ pieces = [] + if args: if len(args) % 2 != 0: - raise RedisError("ZADD requires an equal number of " - "values and scores") + raise RedisError("ZADD requires an equal number of values and scores") pieces.extend(reversed(args)) + for pair in iteritems(kwargs): pieces.append(pair[1]) pieces.append(pair[0]) + return self.execute_command('ZADD', name, *pieces) diff --git a/rediscluster/cluster_mgt.py b/rediscluster/cluster_mgt.py index cfd7ceb1..ed5da5c0 100644 --- a/rediscluster/cluster_mgt.py +++ b/rediscluster/cluster_mgt.py @@ -10,10 +10,12 @@ class RedisClusterMgt(object): - blocked_args = ('addslots', 'count_failure_reports', - 'countkeysinslot', 'delslots', 'failover', 'forget', - 'getkeysinslot', 'keyslot', 'meet', 'replicate', 'reset', - 'saveconfig', 'set_config_epoch', 'setslot', 'slaves') + blocked_args = ( + 'addslots', 'count_failure_reports', + 'countkeysinslot', 'delslots', 'failover', 'forget', + 'getkeysinslot', 'keyslot', 'meet', 'replicate', 'reset', + 'saveconfig', 'set_config_epoch', 'setslot', 'slaves', + ) def __init__(self, startup_nodes=None, **kwargs): self.connection_pool = ClusterConnectionPool( @@ -24,31 +26,37 @@ def __init__(self, startup_nodes=None, **kwargs): def __getattr__(self, attr): if attr in self.blocked_args: raise RedisClusterException('%s is currently not supported' % attr) + raise RedisClusterException('%s is not a valid Redis cluster argument' % attr) @clusterdown_wrapper def _execute_command_on_nodes(self, nodes, *args, **kwargs): command = args[0] res = {} + for node in nodes: c = self.connection_pool.get_connection_by_node(node) + try: c.send_command(*args) res[node["name"]] = c.read_response() except (ConnectionError, TimeoutError) as e: c.disconnect() - if (not c.retry_on_timeout and - isinstance(e, TimeoutError)): + + if not c.retry_on_timeout and isinstance(e, TimeoutError): raise + c.send_command(*args) res[node["name"]] = c.read_response() finally: self.connection_pool.release(c) + return first_key(command, res) def _execute_cluster_commands(self, *args, **kwargs): args = ('cluster',) + args node = self.connection_pool.nodes.random_node() + return self._execute_command_on_nodes([node], *args, **kwargs) def info(self): @@ -58,6 +66,7 @@ def _split(line): k, v = line.split(':') yield k yield v + return {k: v for k, v in [_split(line) for line in raw.split('\r\n') if line]} @@ -68,12 +77,14 @@ def slots(self, host_required=False): slots_info = self._execute_cluster_commands('slots') master_slots = defaultdict(list) slave_slots = defaultdict(list) + for item in slots_info: master_ip, master_port = item[2] slots = [item[0], item[1]] master_host = nslookup(master_ip) if host_required else master_ip master_slots[self._make_host(master_host, master_port)].append(slots) slaves = item[3:] + for slave_ip, slave_port in slaves: slave_host = nslookup(slave_ip) if host_required else slave_ip slave_slots[self._make_host(slave_host, slave_port)].append(slots) @@ -88,17 +99,21 @@ def _parse_node_line(self, line): ret = line_items[:8] slots = [sl.split('-') for sl in line_items[8:]] ret.append(slots) + return ret def nodes(self, host_required=False): raw = self._execute_cluster_commands('nodes') ret = {} + for line in raw.split('\n'): if not line: continue + node_id, ip_port, flags, master_id, ping, pong, epoch, \ status, slots = self._parse_node_line(line) role = flags + if ',' in flags: _, role = flags.split(',') @@ -113,4 +128,5 @@ def nodes(self, host_required=False): 'status': status, 'slots': slots } + return ret diff --git a/rediscluster/connection.py b/rediscluster/connection.py index 2a8fff6f..de63b0b5 100644 --- a/rediscluster/connection.py +++ b/rediscluster/connection.py @@ -50,6 +50,7 @@ def on_connect(self): if self.readonly: self.send_command('READONLY') + if nativestr(self.read_response()) != 'OK': raise ConnectionError('READONLY command failed') @@ -64,7 +65,8 @@ class ClusterConnectionPool(ConnectionPool): """ RedisClusterDefaultTimeout = None - def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=ClusterConnection, max_connections=None, max_connections_per_node=False, **connection_kwargs): + def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=ClusterConnection, + max_connections=None, max_connections_per_node=False, **connection_kwargs): super(ClusterConnectionPool, self).__init__(connection_class=connection_class, max_connections=max_connections) self.max_connections_per_node = max_connections_per_node @@ -85,6 +87,7 @@ def __repr__(self): Return a string with all unique ip:port combinations that this pool is connected to. """ nodes = [{'host': i['host'], 'port': i['port']} for i in self.nodes.startup_nodes] + return "%s<%s>" % ( type(self).__name__, ", ".join([self.connection_class.description_format % dict(node, **self.connection_kwargs) for node in nodes]) @@ -124,6 +127,7 @@ def get_connection(self, command_name, *keys, **options): # TOOD: Pop existing connection if it exists connection = self.make_connection(self.nodes.pubsub_node) self._in_use_pubsub_connections.add(connection) + return connection def make_connection(self, node): @@ -132,6 +136,7 @@ def make_connection(self, node): """ if self.count_num_connections(node) >= self.max_connections: raise RedisClusterException("Too many connections") + self._created_connections += 1 connection = self.connection_class(host=node["host"], port=node["port"], **self.connection_kwargs) @@ -156,6 +161,7 @@ def release(self, connection): # There is cases where the connection is to be removed but it will not exist and there # must be a safe way to remove i_c = self._in_use_connections.get(connection._node["name"], set()) + if connection in i_c: i_c.remove(connection) else: @@ -190,8 +196,10 @@ def count_num_connections(self, node): return len(self._in_use_connections.get(node['name'], [])) i = 0 + for _, connections in self._in_use_connections.items(): i += len(connections) + return i def get_random_connection(self): @@ -202,6 +210,7 @@ def get_random_connection(self): # open available connections and return that instead? for node in self.nodes.random_startup_node_ittr(): connection = self.get_connection_by_node(node) + if connection: return connection @@ -210,6 +219,7 @@ def get_random_connection(self): def get_connection_by_key(self, key): if not key: raise RedisClusterException("No way to dispatch this command to Redis Cluster.") + return self.get_connection_by_slot(self.nodes.keyslot(key)) def get_connection_by_slot(self, slot): @@ -256,7 +266,8 @@ class ClusterReadOnlyConnectionPool(ClusterConnectionPool): Readonly connection pool for rediscluster """ - def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=ClusterConnection, max_connections=None, **connection_kwargs): + def __init__(self, startup_nodes=None, init_slot_cache=True, connection_class=ClusterConnection, + max_connections=None, **connection_kwargs): super(ClusterReadOnlyConnectionPool, self).__init__( startup_nodes=startup_nodes, init_slot_cache=init_slot_cache, diff --git a/rediscluster/nodemanager.py b/rediscluster/nodemanager.py index 033b4f96..b12f05df 100644 --- a/rediscluster/nodemanager.py +++ b/rediscluster/nodemanager.py @@ -34,10 +34,12 @@ def keyslot(self, key): """ k = unicode(key) start = k.find("{") + if start > -1: end = k.find("}", start + 1) if end > -1 and end != start + 1: k = k[start + 1:end] + return crc16(k) % self.RedisClusterHashSlots def all_nodes(self): @@ -51,6 +53,7 @@ def all_masters(self): def random_startup_node(self): random.shuffle(self.startup_nodes) + return self.startup_nodes[0] def random_startup_node_ittr(self): @@ -62,6 +65,7 @@ def random_startup_node_ittr(self): def random_node(self): key = random.choice(list(self.nodes.keys())) + return self.nodes[key] def get_redis_link(self, host, port, decode_responses=False): @@ -122,7 +126,10 @@ def initialize(self): else: # Validate that 2 nodes want to use the same slot cache setup if tmp_slots[i][0]['name'] != node['name']: - disagreements.append("{} vs {} on slot: {}".format(tmp_slots[i][0]['name'], node['name'], i)) + disagreements.append("{} vs {} on slot: {}".format( + tmp_slots[i][0]['name'], node['name'], i), + ) + if len(disagreements) > 5: raise RedisClusterException("startup_nodes could not agree on a valid slots cache. %s" % ", ".join(disagreements)) @@ -163,10 +170,12 @@ def determine_pubsub_node(self): """ highest = -1 node = None + for n in self.nodes.values(): if n["port"] > highest: highest = n["port"] node = n + self.pubsub_node = {"host": node["host"], "port": node["port"], "server_type": node["server_type"], "pubsub": True} def set_node_name(self, n): @@ -209,9 +218,11 @@ def populate_startup_nodes(self): """ for item in self.startup_nodes: self.set_node_name(item) + for n in self.nodes.values(): if n not in self.startup_nodes: self.startup_nodes.append(n) + # freeze it so we can set() it uniq = set([frozenset(node.items()) for node in self.startup_nodes]) # then thaw it back out into a list of dicts diff --git a/rediscluster/pipeline.py b/rediscluster/pipeline.py index bc14a8f6..c8c334dd 100644 --- a/rediscluster/pipeline.py +++ b/rediscluster/pipeline.py @@ -27,18 +27,16 @@ class StrictClusterPipeline(StrictRedisCluster): def __init__(self, connection_pool, nodes_callbacks=None, result_callbacks=None, response_callbacks=None, startup_nodes=None, refresh_table_asap=False, use_threads=True, reinitialize_steps=None): - self.connection_pool = connection_pool - self.startup_nodes = startup_nodes if startup_nodes else [] - self.refresh_table_asap = refresh_table_asap self.command_stack = [] - + self.connection_pool = connection_pool self.nodes_callbacks = nodes_callbacks - self.result_callbacks = result_callbacks - self.response_callbacks = response_callbacks - self.use_threads = use_threads - + self.refresh_table_asap = refresh_table_asap self.reinitialize_counter = 0 self.reinitialize_steps = reinitialize_steps or 25 + self.response_callbacks = response_callbacks + self.result_callbacks = result_callbacks + self.startup_nodes = startup_nodes if startup_nodes else [] + self.use_threads = use_threads def __repr__(self): return "{}".format(type(self).__name__) @@ -76,8 +74,10 @@ def annotate_exception(self, exception, number, command): def execute(self, raise_on_error=True): stack = self.command_stack + if not stack: return [] + try: return self.send_cluster_commands(stack, raise_on_error) finally: @@ -142,6 +142,7 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T for i in attempt: c = stack[i] slot = self._determine_slot(*c[0]) + if slot in ask_slots: node = ask_slots[slot] else: @@ -162,6 +163,7 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T attempt = [] if self.use_threads and len(node_commands) > 1: workers = dict() + for node_name in node_commands: node = nodes[node_name] cmds = [node_commands[node_name][i] for i in sorted(node_commands[node_name].keys())] @@ -175,6 +177,7 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T for node_name, worker in workers.items(): worker.join() + if worker.exception: for i in sorted(node_commands[node_name].keys()): response[i] = worker.exception @@ -201,6 +204,7 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T if isinstance(v, ConnectionError): ask_slots[self.connection_pool.nodes.keyslot(stack[i][0][1])] = random.choice(self.startup_nodes) attempt.append(i) + if ttl < self.RedisClusterRequestTTL / 2: time.sleep(0.1) continue @@ -211,11 +215,13 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T self.connection_pool.disconnect() self.connection_pool.reset() self.refresh_table_asap = True + raise v if isinstance(v, MovedError): # Do not perform full cluster refresh on every MOVED error self.reinitialize_counter += 1 + if self.reinitialize_counter % self.reinitialize_steps == 0: self.refresh_table_asap = True @@ -230,6 +236,7 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T self._fail_on_redirect(allow_redirections) response = [response[k] for k in sorted(response.keys())] + if raise_on_error: self.raise_first_error(stack, response) @@ -247,12 +254,14 @@ def _execute_pipeline(self, connection, commands, raise_on_error): """ # build up all commands into a single request to increase network perf all_cmds = connection.pack_commands([args for args, _ in commands]) + try: connection.send_packed_command(all_cmds) except ConnectionError as e: return [e for _ in xrange(len(commands))] # noqa response = [] + for args, options in commands: try: response.append(self.parse_response(connection, args[0], **options)) @@ -261,6 +270,7 @@ def _execute_pipeline(self, connection, commands, raise_on_error): if raise_on_error: self.raise_first_error(commands, response) + return response def multi(self): @@ -300,6 +310,7 @@ def block_pipeline_command(func): """ def inner(*args, **kwargs): raise RedisClusterException("ERROR: Calling pipelined function {} is blocked when running redis in cluster mode...".format(func.__name__)) + return inner @@ -370,6 +381,7 @@ class ThreadedPipelineExecute(threading.Thread): Threaded pipeline execution. release the connection back into the pool after executing. """ + def __init__(self, execute, pool, node, cmds): threading.Thread.__init__(self) self.execute = execute diff --git a/rediscluster/pubsub.py b/rediscluster/pubsub.py index 6e7c1a05..4e3e6956 100644 --- a/rediscluster/pubsub.py +++ b/rediscluster/pubsub.py @@ -8,5 +8,6 @@ class ClusterPubSub(PubSub): """ Wrapper for PubSub class. """ + def __init__(self, *args, **kwargs): super(ClusterPubSub, self).__init__(*args, **kwargs) diff --git a/rediscluster/utils.py b/rediscluster/utils.py index 36468390..8eec1400 100644 --- a/rediscluster/utils.py +++ b/rediscluster/utils.py @@ -12,6 +12,7 @@ def is_dict(d): Test if variable is a dict or not. """ assert isinstance(d, dict) + return True @@ -28,9 +29,11 @@ def dict_merge(*dicts): Merge all provided dicts into 1 dict. """ merged = {} + for d in dicts: if is_dict(d): merged.update(d) + return merged @@ -51,9 +54,11 @@ def merge_result(command, res): is_dict(res) result = set([]) + for _, v in res.items(): for value in v: result.add(value) + return list(result) @@ -67,6 +72,7 @@ def first_key(command, res): if len(res.keys()) != 1: raise RedisClusterException("More then 1 result from command: {0}".format(command)) + return list(res.values())[0] @@ -98,5 +104,7 @@ def inner(*args, **kwargs): def nslookup(node_ip): if ':' not in node_ip: return gethostbyaddr(node_ip)[0] + ip, port = node_ip.split(':') + return '%s:%s' % (gethostbyaddr(ip)[0], port) diff --git a/tests/test_cluster_connection_pool.py b/tests/test_cluster_connection_pool.py index e17dfa4a..feb78c8d 100644 --- a/tests/test_cluster_connection_pool.py +++ b/tests/test_cluster_connection_pool.py @@ -33,7 +33,8 @@ def __init__(self, host="localhost", port=7000, socket_timeout=None, **kwargs): class TestConnectionPool(object): - def get_pool(self, connection_kwargs=None, max_connections=None, max_connections_per_node=None, connection_class=DummyConnection, init_slot_cache=True): + def get_pool(self, connection_kwargs=None, max_connections=None, max_connections_per_node=None, + connection_class=DummyConnection, init_slot_cache=True): connection_kwargs = connection_kwargs or {} pool = ClusterConnectionPool( connection_class=connection_class, From 739da4917bb349f0fe34fc76e9548a05c0f4ee68 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sun, 18 Oct 2015 19:31:50 +0200 Subject: [PATCH 38/50] Update readme regarding supported python versions --- README.md | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 48a9cb34..e3156209 100644 --- a/README.md +++ b/README.md @@ -38,11 +38,16 @@ Supported python versions, all minor releases in each major version should be su - 3.2.x - 3.3.x - 3.4.1+ -- 3.5.0 (Beta 4) +- 3.5.0 + +Experimental: + +- Python 3.6.0a0 - Currently broken due to `coverage` is not yet compatible with python 3.6 -Python 3.4.0 do not not work with pubsub because of segfault issues (Same as redis-py has). If rediscluster is runned on 3.4.0 it will raise RuntimeError exception and exit. If you get this error locally when running tox, consider using `pyenv` to fix this problem. -Testing with python 3.5.0 Beta 4 on travis-ci to ensure it will work when it is released in a stable version. +### Python 3.4.0 + +Python 3.4.0 do not not work with pubsub because of segfault issues (Same as redis-py has). If rediscluster is runned on 3.4.0 it will raise RuntimeError exception and exit. If you get this error locally when running tox, consider using `pyenv` to fix this problem. From 4a2d4d71d357d2b424f897cebae3f7f5648e8d49 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Sun, 18 Oct 2015 18:35:51 +0200 Subject: [PATCH 39/50] Fix ASKING error handling by now sending ASKING to next node --- CHANGES | 1 + rediscluster/client.py | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index e7598d3e..79552027 100644 --- a/CHANGES +++ b/CHANGES @@ -8,6 +8,7 @@ * Added support for SCRIPT LOAD, SCRIPT FLUSH, SCRIPT EXISTS and EVALSHA commands. (alisaifee) * Improve thread safty to avoid exceptions when running one client object inside multiple threads and doing resharding of the cluster at the same time. + * Fix ASKING error handling so now it really sends ASKING to next node during a reshard operation * 1.0.0 * No change to anything just a bump to 1.0.0 because the lib is now considered stable/production ready. diff --git a/rediscluster/client.py b/rediscluster/client.py index 0334d42d..ee4b659d 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -260,6 +260,10 @@ def execute_command(self, *args, **kwargs): r = self.connection_pool.get_connection_by_node(node) try: + if asking: + r.send_command('ASKING') + asking = False + r.send_command(*args) return self.parse_response(r, command, **kwargs) except (RedisClusterException, BusyLoadingError): From 199ee0b904f8b9e3480436f59180ebdfe3c88fb3 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Thu, 22 Oct 2015 10:11:55 +0200 Subject: [PATCH 40/50] Fix broken ASKING impl in regular client --- rediscluster/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rediscluster/client.py b/rediscluster/client.py index ee4b659d..ec82b820 100644 --- a/rediscluster/client.py +++ b/rediscluster/client.py @@ -262,6 +262,7 @@ def execute_command(self, *args, **kwargs): try: if asking: r.send_command('ASKING') + self.parse_response(r, "ASKING", **kwargs) asking = False r.send_command(*args) @@ -295,7 +296,6 @@ def execute_command(self, *args, **kwargs): if ttl < self.COMMAND_TTL / 2: time.sleep(0.05) except AskError as e: - node = self.connection_pool.nodes.set_node(e.host, e.port, server_type='master') redirect_addr, asking = "%s:%s" % (e.host, e.port), True finally: self.connection_pool.release(r) From 3294e76c923a7f2ba4989295ba8489ae2b3ca952 Mon Sep 17 00:00:00 2001 From: John Loehrer Date: Fri, 23 Oct 2015 06:55:31 -0700 Subject: [PATCH 41/50] handle ASK error responses correctly in pipeline context. better comments for logic in pipeline. --- rediscluster/pipeline.py | 230 ++++++++++++++++++++++++++++++++------- 1 file changed, 193 insertions(+), 37 deletions(-) diff --git a/rediscluster/pipeline.py b/rediscluster/pipeline.py index c8c334dd..fb5f04cd 100644 --- a/rediscluster/pipeline.py +++ b/rediscluster/pipeline.py @@ -129,29 +129,64 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T self.refresh_table_asap = False ttl = self.RedisClusterRequestTTL + + # this is where we store all the responses to the pipeline execute. + # the actual response is a list, not a dict, but because we may be getting + # the responses for commands out of order or sometimes retrying them it is important + # that we have a way to key the responses by the order they came in so that we can return + # the responses as if we did them sequentially. response = {} + + # `attempt` corresponds to the sequence number of commands still left to process or retry. + # initially this corresponds exactly to the number of commands we need to run, and if all goes + # well, that's where it'll end. Everything will be attempted once, and we end up with an empty + # array of commands left to process. But if we need to retry, `attempt` gets repopulated with + # the sequence number of the command that is being retried. attempt = xrange(0, len(stack)) if stack else [] # noqa - ask_slots = {} + # there are different types of retries. redis cluster says if it responds with an ASK error, + # you need to handle it differently than a moved error. And if we hit a connection error, we + # don't really know what to do for that command so we pick a random other node in the cluster. + ask_retry = {} + conn_retry = {} + + # as long as we have commands left to attempt and we haven't overrun the max attempts, keep trying. while attempt and ttl > 0: + + # decrement our counter for number of attempts left before giving up. ttl -= 1 + + # each time we go through this we need to separate out the commands by node. node_commands = {} + + # build a list of node objects based on node names we need to nodes = {} - # Keep this section so that we can determine what nodes to contact + # as we move through each command that still needs to be processed, + # we figure out the slot number that command maps to, then from the slot determine the node. for i in attempt: c = stack[i] slot = self._determine_slot(*c[0]) - if slot in ask_slots: - node = ask_slots[slot] + # normally we just refer to our internal node -> slot table that tells us where a given + # command should route to. + # but if we are retrying, the cluster could have told us we were wrong or the node was down. + # in that case, we have to try something that contradicts our rules. + if i in ask_retry: + node = ask_retry[i] + elif i in conn_retry: + node = conn_retry[i] else: if self.refresh_table_asap: # MOVED node = self.connection_pool.get_master_node_by_slot(slot) else: node = self.connection_pool.get_node_by_slot(slot) + # little hack to make sure the node name is populated. probably could clean this up. self.connection_pool.nodes.set_node_name(node) + + # now that we know the name of the node ( it's just a string in the form of host:port ) + # we can build a list of commands for each node. node_name = node['name'] nodes[node_name] = node node_commands.setdefault(node_name, {}) @@ -160,51 +195,162 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T # Get one connection at a time from the pool and basiccly copy the logic from # _execute_pipeline() below and do what a normal pipeline does. + # now that we've split out the commands by the node we plan to send them to, + # we can reset the commands we'll attempt next time around back to nothing. + # when we process the response, any commands that need to be retried because of + # connection errors, MOVED errors, or ASKING errors will be dumped into here. + # most of the time this array will just stay empty. attempt = [] - if self.use_threads and len(node_commands) > 1: - workers = dict() - - for node_name in node_commands: - node = nodes[node_name] - cmds = [node_commands[node_name][i] for i in sorted(node_commands[node_name].keys())] - - workers[node_name] = ThreadedPipelineExecute( - execute=self._execute_pipeline, - pool=self.connection_pool, - node=node, - cmds=cmds) - workers[node_name].start() - for node_name, worker in workers.items(): - worker.join() + # only use threads when it makes sense performance-wise to do so. + # if you are doing an ask_retry, explicitly disable it. + # makes it easier to define the logic of how to do the ASKING command, + # and in practice, the server will only be moving one slot at a time, + # so there will only be one server that will be recieving these ASKING retries + # anyway. + if self.use_threads and not ask_retry and len(node_commands) > 1: + + # for each node we query, we need to have one worker. + # that way all pipelined commands are issued in parallel. + # this could be a problem if you have hundreds of nodes in your cluster. + # We should really refactor this to be a thread pool of workers, and allocate up to a + # certain max number of threads. + workers = dict() - if worker.exception: - for i in sorted(node_commands[node_name].keys()): - response[i] = worker.exception - else: - for i, v in zip(sorted(node_commands[node_name].keys()), worker.value): - response[i] = v - del workers + # allocate all of the redis connections from the connection pool. + # each connection gets passed into its own thread so it can query each node in paralle. + connections = {node_name: self.connection_pool.get_connection_by_node(nodes[node_name]) + for node_name in node_commands} + + # iterate through each set of commands and pass them into a worker thread so + # it can be executed in it's own socket connection from the redis connection pool + # in parallel. + try: + + for node_name in node_commands: + node = nodes[node_name] + # build the list of commands to be passed to a particular node. + # we have to do this on each attempt, because the cluster may respond to us + # that a command for a given slot actually should be routed to a different node. + cmds = [node_commands[node_name][i] for i in sorted(node_commands[node_name].keys())] + + # pass all the commands bound for a particular node into a thread worker object + # along with the redis connection needed to run the commands and parse the response. + workers[node_name] = ThreadedPipelineExecute( + execute=self._execute_pipeline, + conn=connections[node_name], + cmds=cmds) + + workers[node_name].start() + + # now that all the queries are running against all the nodes, + # wait for all of them to come back so we can parse the responses. + for node_name, worker in workers.items(): + worker.join() + + # if the worker hit an exception this is really bad. + # that means something completely unexpected happened. + # we have to assume the worst and assume that all the calls against + # that particular node in the cluster failed and will need to be retried. + # maybe that isn't a safe assumption? + if worker.exception: + for i in node_commands[node_name].keys(): + response[i] = worker.exception + else: + # we got a valid response back from redis. + # map each response based on the sequence of the original request stack. + # some of these responses may represent redis connection or ask errors etc. + for i, v in zip(sorted(node_commands[node_name].keys()), worker.value): + response[i] = v + + + finally: + # don't need our threads anymore. + # explicitly remove them from the current namespace so they can be garbage collected. + del workers + + # release all of the redis connections we allocated earlier back into the connection pool. + for conn in connections.values(): + self.connection_pool.release(conn) + del connections else: + # if we got here, it's because threading is disabled explicitly, or + # all the commands map to a single node so we don't need to use different threads to + # issue commands in parallel. + + # first, we need to keep track of all the commands and what responses they map to. + # this is because we need to interject ASKING commands into the mix. I thought of a little + # hack to map these responses back to None instead of the integer sequence id that was the + # position number of the command issued in the stack of command requests at the point pipeline + # execute was issued. + track_cmds = {} + + # send the commands in sequence. for node_name in node_commands: node = nodes[node_name] - cmds = [node_commands[node_name][i] for i in sorted(node_commands[node_name].keys())] + cmds = [] + track_cmds[node_name] = [] + + # we've got the commands we need to run for each node, + # sort them to make sure that they are executed in the same order + # they came in on the stack otherwise it changes the logic. + # we make no guarantees about the order of execution of the commands run + # except that we are sure we will always process the commands for a given key + # in a sequential order. If we get an error from the server about a given key, + # that will apply the same for all commands on that key (MOVED, ASKING, etc) + # so we will be resending all of the commands for that key to a new node. + for i in sorted(node_commands[node_name].keys()): + if i in ask_retry: + # put in our fake stub placeholder for the response. + track_cmds[node_name].append(None) + cmds.append((['ASKING'], {})) + + # keep track of the sequence number and the mapping of actual commands + # sent to the node. (ASKING SCREWS EVERYTHING UP!!!!!) + track_cmds[node_name].append(i) + cmds.append(node_commands[node_name][i]) + + # allocate a connection from the connection pool and send the commands for each node + # as a packed single network request. Since we aren't using threads here, we are + # only able to send each request sequentially and block, waiting for the response. + # After we get the response to one connection, we move on to the next. with by_node_context(self.connection_pool, node) as connection: result = zip( - sorted(node_commands[node_name].keys()), + track_cmds[node_name], self._execute_pipeline(connection, cmds, False)) + + # map the response from the connection to the commands we were running. for i, v in result: - response[i] = v - ask_slots = {} + # remember None is a shim value used above as a placeholder for the ASKING + # command. That response is just `OK` and we don't care about that. + # throw it away. + # Map the response here to the original sequence of commands in the stack + # sent to pipeline. + if i is not None: + response[i] = v + + ask_retry = {} + conn_retry = {} + + # now that we have tried to execute all the commands let's see what we have left. for i, v in response.items(): + # if the response isn't an exception it is a valid response from the node + # we're all done with that command, YAY! + # if we move beyond this point, we've run into problems and we need to retry the command. if not isinstance(v, Exception): continue + # connection errors are tricky because most likely we routed to the right node but it is + # down. In that case, the best we can do is randomly try another node in the cluster + # and hope that it tells us to try that node again with a MOVED error or tells us the new + # master. if isinstance(v, ConnectionError): - ask_slots[self.connection_pool.nodes.keyslot(stack[i][0][1])] = random.choice(self.startup_nodes) + conn_retry[i] = random.choice(self.startup_nodes) attempt.append(i) + # if we are stuck in a retry loop, slow things down a bit to give the failover + # a chance of actually happening. if ttl < self.RedisClusterRequestTTL / 2: time.sleep(0.1) continue @@ -218,6 +364,10 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T raise v + # A MOVED response from the cluster means that somehow we were misinformed about which node + # a given key slot maps to. This can happen during cluster resharding, during master-slave + # failover, or if we got a connection error and were forced to re-attempt the command against a + # random node. if isinstance(v, MovedError): # Do not perform full cluster refresh on every MOVED error self.reinitialize_counter += 1 @@ -229,12 +379,20 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T self.connection_pool.nodes.slots[v.slot_id][0] = node attempt.append(i) self._fail_on_redirect(allow_redirections) + + # an ASK error from the server means that only this specific command needs to be tried against + # a different server (not every key in the slot). This happens only during cluster re-sharding + # and is a major pain in the ass. For it to work correctly, we have to resend the command to + # the new node but only after first sending an ASKING command immediately beforehand. elif isinstance(v, AskError): node = self.connection_pool.nodes.set_node(v.host, v.port, server_type='master') - ask_slots[v.slot_id] = node + ask_retry[i] = node attempt.append(i) self._fail_on_redirect(allow_redirections) + # YAY! we made it out of the attempt loop. + # turn the response back into a simple flat array that corresponds + # to the sequence of commands issued in the stack in pipeline.execute() response = [response[k] for k in sorted(response.keys())] if raise_on_error: @@ -382,18 +540,16 @@ class ThreadedPipelineExecute(threading.Thread): release the connection back into the pool after executing. """ - def __init__(self, execute, pool, node, cmds): + def __init__(self, execute, conn, cmds): threading.Thread.__init__(self) self.execute = execute - self.pool = pool - self.node = node + self.conn = conn self.cmds = cmds self.exception = None self.value = None def run(self): try: - with by_node_context(self.pool, self.node) as connection: - self.value = self.execute(connection, self.cmds, False) + self.value = self.execute(self.conn, self.cmds, False) except Exception as e: self.exception = e From c01efe7cd94cd92a513f039f80d8fa4c5087effb Mon Sep 17 00:00:00 2001 From: John Loehrer Date: Sat, 24 Oct 2015 20:10:25 -0700 Subject: [PATCH 42/50] improvements to docs on pipelining --- CHANGES | 5 +++-- docs/Pipelines.md | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/CHANGES b/CHANGES index 79552027..d896b86d 100644 --- a/CHANGES +++ b/CHANGES @@ -6,9 +6,10 @@ * Improve thread safty of get_connection_by_slot and get_connection_by_node methods (iandyh) * Improved error handling when sending commands to all nodes, e.g. info. Now the connection takes retry_on_timeout as an option and retry once when there is a timeout. (iandyh) * Added support for SCRIPT LOAD, SCRIPT FLUSH, SCRIPT EXISTS and EVALSHA commands. (alisaifee) - * Improve thread safty to avoid exceptions when running one client object inside multiple threads and doing resharding of the + * Improve thread safety to avoid exceptions when running one client object inside multiple threads and doing resharding of the cluster at the same time. - * Fix ASKING error handling so now it really sends ASKING to next node during a reshard operation + * Fix ASKING error handling so now it really sends ASKING to next node during a reshard operation. This improvement was also made to pipelined commands. + * Improved thread safety in pipelined commands, along better explanation of the logic inside pipelining with code comments. * 1.0.0 * No change to anything just a bump to 1.0.0 because the lib is now considered stable/production ready. diff --git a/docs/Pipelines.md b/docs/Pipelines.md index 386b26f0..d5fd818c 100644 --- a/docs/Pipelines.md +++ b/docs/Pipelines.md @@ -1,3 +1,22 @@ +# How pipelining works + +Just like in redis-py, redis-py-cluster queues up all the commands inside the client until execute is called. But, once execute is called, redis-py-cluster internals work slightly differently. It still packs the commands to efficiently +transmit multiple commands across the network. But since different keys may be mapped to different nodes, redis-py-cluster must first map each key to the expected node. It then packs all the commands destined for each node in the cluster into its own packed sequence of commands. It uses the redis-py library to communicate with each node in the cluster. +Ideally all the commands should be sent to each node in the cluster in parallel so that all the commands can be processed as fast as possible. The naive approach is to iterate through each node and send each batch of commands sequentially to each node. If redis-py supported some sort of non-blocking i/o we could send the network requests first +and multiplex the socket responses from each node. Instead, we use threads to send the requests in parallel so that the total execution time only equals the amount of time for the slowest round trip to and from the given set of nodes in the cluster needed to process the commands. +In previous versions of the library there were some bugs associated with threaded operations and pipelining. We were freeing connections back into the connection pool prior to reading the responses from each thread and it caused all kinds of problems. But these issues should now be addressed. Ff you are worried, you can disable the threaded behavior with a flag so that it only sends the commands to each node sequentially. + + +# Connection Error handling + +The other way pipelines differ in redis-py-cluster from redis-py is in error handling and retries. With the normal redis-py client, if you hit a connection error during a pipeline command it raises the error right there. But we expect redis-cluster to be more resilient to failures. If you hit a connection problem with one of the nodes in the cluster, most likely a stand-by slave will take over for the down master pretty quickly. In this case, we try the commands bound for that particular node to another random node. The other random node will not just blindly accept these commands. It only accepts them if the keys referenced in those commands actually map to that node in the cluster configuration. So most likely it will respond with a MOVED error telling the client the new master for those commands. Our code handles these MOVED commands according to the redis cluster specification and re-issues the commands to the correct server transparently inside of pipeline.execute(). You can disable this behavior if you'd like as well. + + +# ASKED and MOVED errors + +The other tricky part of the redis-cluster specification is that if any command response comes back with an ASK or MOVED error, the command is to be retried against the specified node. In previous versions of redis-py-cluster we were treating ASKED and MOVED errors the same, but they really need to be handled differently. MOVED error means that the client can safely update its own representation of the slots table to point to a new node for all future commands bound for that slot. But an ASK error means the slot is only partially migrated and that the client can only successfully issue that command to the new server if it prefixes the request with an ASKING command first. This lets the new node taking over that slot know that the original server said it was okay to run that command for the given key against the new node even though the slot is not yet completely migrated. Our current implementation now handles this case correctly. + + # The philosophy on pipelines After playing around with pipelines and thinking about possible solutions that could be used in a cluster setting this document will describe how pipelines work, strengths and weaknesses with the implementation that was chosen. From 8e2790eb9b588c057e45ff1de9fb30e2a98863e6 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Mon, 26 Oct 2015 15:28:31 +0100 Subject: [PATCH 43/50] Add 2 new example scripts --- examples/README.md | 3 +++ examples/incr-test-writer.py | 9 +++++++++ examples/pipeline-incrby.py | 20 ++++++++++++++++++++ 3 files changed, 32 insertions(+) create mode 100644 examples/README.md create mode 100644 examples/incr-test-writer.py create mode 100644 examples/pipeline-incrby.py diff --git a/examples/README.md b/examples/README.md new file mode 100644 index 00000000..ee3c70dd --- /dev/null +++ b/examples/README.md @@ -0,0 +1,3 @@ +In this folder, you will find some example scripts that will both demonstrate some examples on how to use certain functionality and it also function as test scripts to ensure the client works as expected during usage of those functionalities. + +To really ensure the scripts is working, they should be runned during a resharding operation to ensure that all redirection and fault handling code works without throwing any unexpected errors. diff --git a/examples/incr-test-writer.py b/examples/incr-test-writer.py new file mode 100644 index 00000000..0d9e64c0 --- /dev/null +++ b/examples/incr-test-writer.py @@ -0,0 +1,9 @@ +from rediscluster import RedisCluster + +startup_nodes = [{"host": "127.0.0.1", "port": 7000}] +r = RedisCluster(startup_nodes=startup_nodes, max_connections=32, decode_responses=True) + +for i in xrange(1000000): + d = str(i) + r.set(d, d) + r.incrby(d, 1) diff --git a/examples/pipeline-incrby.py b/examples/pipeline-incrby.py new file mode 100644 index 00000000..71c4f0e0 --- /dev/null +++ b/examples/pipeline-incrby.py @@ -0,0 +1,20 @@ +from rediscluster import RedisCluster + +startup_nodes = [{"host": "127.0.0.1", "port": 7000}] +r = RedisCluster(startup_nodes=startup_nodes, max_connections=32, decode_responses=True) + +for i in xrange(1000000): + d = str(i) + pipe = r.pipeline(transaction=False) + pipe.set(d, d) + pipe.incrby(d, 1) + pipe.execute() + + pipe = r.pipeline(transaction=False) + pipe.set("foo-%s" % d, d) + pipe.incrby("foo-%s" % d, 1) + pipe.set("bar-%s" % d, d) + pipe.incrby("bar-%s" % d, 1) + pipe.set("bazz-%s" % d, d) + pipe.incrby("bazz-%s" % d, 1) + pipe.execute() From a50f08b53023f7487fa6b8968668ae089a0f7672 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 27 Oct 2015 00:59:13 +0100 Subject: [PATCH 44/50] Code style fixes --- rediscluster/pipeline.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rediscluster/pipeline.py b/rediscluster/pipeline.py index fb5f04cd..186cbbf9 100644 --- a/rediscluster/pipeline.py +++ b/rediscluster/pipeline.py @@ -219,8 +219,10 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T # allocate all of the redis connections from the connection pool. # each connection gets passed into its own thread so it can query each node in paralle. - connections = {node_name: self.connection_pool.get_connection_by_node(nodes[node_name]) - for node_name in node_commands} + connections = { + node_name: self.connection_pool.get_connection_by_node(nodes[node_name]) + for node_name in node_commands + } # iterate through each set of commands and pass them into a worker thread so # it can be executed in it's own socket connection from the redis connection pool @@ -262,8 +264,6 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T # some of these responses may represent redis connection or ask errors etc. for i, v in zip(sorted(node_commands[node_name].keys()), worker.value): response[i] = v - - finally: # don't need our threads anymore. # explicitly remove them from the current namespace so they can be garbage collected. From adcdb4f38ba5ef8e3c1daa5c3531c380dd2933d3 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 27 Oct 2015 01:09:16 +0100 Subject: [PATCH 45/50] Add a few lines to Upgrading instructions --- docs/Upgrading.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/Upgrading.md b/docs/Upgrading.md index fcd3494c..560ddd69 100644 --- a/docs/Upgrading.md +++ b/docs/Upgrading.md @@ -24,6 +24,10 @@ Added optional `max_connections_per_node` parameter to `ClusterConnectionPool` w Reinitialize on `MOVED` errors will not run on every error but instead on every 25 error to avoid excessive cluster reinitialize when used in multiple threads and resharding at the same time. If you want to go back to the old behaviour with reinitialize on every error you should pass in `reinitialize_steps=1` to the client constructor. If you want to increase or decrease the intervall of this new behaviour you should set `reinitialize_steps` in the client constructor to a value that you want. +Pipelines in general have recieved alot of attention so if you are using pipelines in your code, ensure that you test the new code out alot before using it to make sure it still works as you expect. + +The entire client code should now be safer to use in a threaded environment. Some race conditions was found and have now been fixed and it should prevent the code from behaving wierd during reshard operations. + ## 0.2.0 --> 0.3.0 From cd3bf40eec360c632fb152b8e421d554a0a074fe Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 27 Oct 2015 01:12:20 +0100 Subject: [PATCH 46/50] Move a few sections further down to make it look more pretty --- README.md | 66 +++++++++++++++++++++++++++---------------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index e3156209..af4a2b7b 100644 --- a/README.md +++ b/README.md @@ -18,39 +18,6 @@ On the topic about porting/moving this code into `redis-py` there is currently w -## Upgrading instructions - -Please read the [following](docs/Upgrading.md) documentation that will go through all changes that is required when upgrading `redis-py-cluster` between versions. - - - -## Dependencies & supported python versions - -- Python: redis >= `2.10.2` is required -- Redis server >= `3.0.0` is required -- Optional Python: hiredis >= `0.1.3` - -Hiredis is tested and supported on all supported python versions. - -Supported python versions, all minor releases in each major version should be supported unless otherwise stated here: - -- 2.7.x -- 3.2.x -- 3.3.x -- 3.4.1+ -- 3.5.0 - -Experimental: - -- Python 3.6.0a0 - Currently broken due to `coverage` is not yet compatible with python 3.6 - - -### Python 3.4.0 - -Python 3.4.0 do not not work with pubsub because of segfault issues (Same as redis-py has). If rediscluster is runned on 3.4.0 it will raise RuntimeError exception and exit. If you get this error locally when running tox, consider using `pyenv` to fix this problem. - - - ## Installation Latest stable release from pypi @@ -92,6 +59,39 @@ The following imports can be imported from `redis` package. +## Upgrading instructions + +Please read the [following](docs/Upgrading.md) documentation that will go through all changes that is required when upgrading `redis-py-cluster` between versions. + + + +## Dependencies & supported python versions + +- Python: redis >= `2.10.2` is required +- Redis server >= `3.0.0` is required +- Optional Python: hiredis >= `0.1.3` + +Hiredis is tested and supported on all supported python versions. + +Supported python versions, all minor releases in each major version should be supported unless otherwise stated here: + +- 2.7.x +- 3.2.x +- 3.3.x +- 3.4.1+ +- 3.5.0 + +Experimental: + +- Python 3.6.0a0 - Currently broken due to `coverage` is not yet compatible with python 3.6 + + +### Python 3.4.0 + +Python 3.4.0 do not not work with pubsub because of segfault issues (Same as redis-py has). If rediscluster is runned on 3.4.0 it will raise RuntimeError exception and exit. If you get this error locally when running tox, consider using `pyenv` to fix this problem. + + + ## Testing All tests are currently built around a 6 redis server cluster setup (3 masters + 3 slaves). One server must be using port 7000 for redis cluster discovery. From fb269b6ac7e55b9e9c78595aa3bebc35a2887de6 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 27 Oct 2015 01:19:33 +0100 Subject: [PATCH 47/50] Simplify usage example section --- README.md | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index af4a2b7b..9300dd4b 100644 --- a/README.md +++ b/README.md @@ -36,27 +36,22 @@ $ python setup.py install ## Usage example -Small sample script that show how to get started with RedisCluster. `decode_responses=True` is required to have when running on python3. +Small sample script that shows how to get started with RedisCluster. It can also be found in [examples/basic.py](examples/basic.py) ```python >>> from rediscluster import StrictRedisCluster + >>> startup_nodes = [{"host": "127.0.0.1", "port": "7000"}] + +>>> # Note: decode_responses must be set to True when used with python3 >>> rc = StrictRedisCluster(startup_nodes=startup_nodes, decode_responses=True) + >>> rc.set("foo", "bar") True ->>> rc.get("foo") +>>> print(rc.get("foo")) 'bar' ``` -The following imports can be imported from `redis` package. - -- `StrictRedisCluster` -- `RedisCluster` -- `StrictClusterPipeline` -- `ClusterPubSub` - -`StrictRedisCluster` is based on `redis.StrictRedis` and `RedisCluster` has the same functionality as `redis.Redis` even if it is not directly based on it. - ## Upgrading instructions From 8dcd9b1ba127d510bf3008cf182dd6c5484c3c6c Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 27 Oct 2015 01:34:51 +0100 Subject: [PATCH 48/50] Rewrite some doc in Readme to make it easier to read --- README.md | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 9300dd4b..41b98077 100644 --- a/README.md +++ b/README.md @@ -1,20 +1,20 @@ # redis-py-cluster -Redis cluster client in python for the official cluster support targeted for redis 3.0. +This client provides a working client for redis cluster that was added in redis 3.0. This project is a port of `redis-rb-cluster` by antirez, with alot of added functionality. The original source can be found at https://github.com/antirez/redis-rb-cluster -[![Build Status](https://travis-ci.org/Grokzen/redis-py-cluster.svg?branch=master)](https://travis-ci.org/Grokzen/redis-py-cluster) [![Coverage Status](https://coveralls.io/repos/Grokzen/redis-py-cluster/badge.png)](https://coveralls.io/r/Grokzen/redis-py-cluster) [![PyPI version](https://badge.fury.io/py/redis-py-cluster.svg)](http://badge.fury.io/py/redis-py-cluster) [![Gitter](https://badges.gitter.im/Join Chat.svg)](https://gitter.im/Grokzen/redis-py-cluster?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge) [![Code Health](https://landscape.io/github/Grokzen/redis-py-cluster/unstable/landscape.svg)](https://landscape.io/github/Grokzen/redis-py-cluster/unstable) +[![Build Status](https://travis-ci.org/Grokzen/redis-py-cluster.svg?branch=master)](https://travis-ci.org/Grokzen/redis-py-cluster) [![Coverage Status](https://coveralls.io/repos/Grokzen/redis-py-cluster/badge.png)](https://coveralls.io/r/Grokzen/redis-py-cluster) [![PyPI version](https://badge.fury.io/py/redis-py-cluster.svg)](http://badge.fury.io/py/redis-py-cluster) [![Code Health](https://landscape.io/github/Grokzen/redis-py-cluster/unstable/landscape.svg)](https://landscape.io/github/Grokzen/redis-py-cluster/unstable) # Project status -The project is not dead but not much new development is done right now. I do awnser issue reports and pull requests as soon as possible and if you have a problem you can ping me inside the gitter channel that you can find here [![Gitter](https://badges.gitter.im/Join Chat.svg)](https://gitter.im/Grokzen/redis-py-cluster?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge) and i will help you out with problems or usage of this lib. +The project is not dead but, not much new development is done right now. I do answer issue reports and pull requests as soon as possible. If you have a problem with the code, you can ping me inside the gitter channel that you can find here [![Gitter](https://badges.gitter.im/Join Chat.svg)](https://gitter.im/Grokzen/redis-py-cluster?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge) and i will help you out with problems or usage of this lib. -As of release `0.3.0` this project will be considered stable and usable in production. Just remember that if you are going to use redis cluster to please read up on the documentation that you can find in the bottom of this Readme. It will contain usage examples and descriptions of what is implemented and what is not implemented and why things are the way they are. +As of release `0.3.0` this project will be considered stable and usable in production. If you are going to use redis cluster in your project, you should read up on all documentation that you can find in the bottom of this Readme file. It will contain usage examples and descriptions of what is and what is not implemented. It will also describe how and why things work the way they do in this client. -On the topic about porting/moving this code into `redis-py` there is currently work over here https://github.com/andymccurdy/redis-py/pull/604 that will bring cluster uspport based on this code. But my suggestion is that until that work is completed that you should use this lib. +On the topic about porting/moving this code into `redis-py` there is currently work over here https://github.com/andymccurdy/redis-py/pull/604 that will bring cluster support based on this code. But my suggestion is that until that work is completed that you should use this lib. @@ -66,15 +66,15 @@ Please read the [following](docs/Upgrading.md) documentation that will go throug - Redis server >= `3.0.0` is required - Optional Python: hiredis >= `0.1.3` -Hiredis is tested and supported on all supported python versions. +Hiredis is tested on all supported python versions. -Supported python versions, all minor releases in each major version should be supported unless otherwise stated here: +List of all supported python versions. -- 2.7.x -- 3.2.x -- 3.3.x +- 2.7 +- 3.2 +- 3.3 - 3.4.1+ -- 3.5.0 +- 3.5 Experimental: @@ -83,7 +83,7 @@ Experimental: ### Python 3.4.0 -Python 3.4.0 do not not work with pubsub because of segfault issues (Same as redis-py has). If rediscluster is runned on 3.4.0 it will raise RuntimeError exception and exit. If you get this error locally when running tox, consider using `pyenv` to fix this problem. +A segfault was found when running `redis-py` in python `3.4.0` that was introduced into the codebase in python `3.4.0`. Because of this both `redis-py` and `redis-py-cluster` will not work when running with `3.4.0`. This lib has decided to block the lib from execution on `3.4.0` and you will get a exception when trying to import the code. The only solution is to use python `3.4.1` or some other higher minor version in the `3.4` series. From b645e0e5678b3adaa27ec7bc4d22f6625e674079 Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 27 Oct 2015 01:38:38 +0100 Subject: [PATCH 49/50] Bump version number to prepare for release --- CHANGES | 2 +- docs/Upgrading.md | 2 +- rediscluster/__init__.py | 2 +- setup.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGES b/CHANGES index d896b86d..ae7ecf54 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,4 @@ -* Next release +* 1.1.0 * Refactored exception handling and exception classes. * Added READONLY mode support, scales reads using slave nodes. * Fix __repr__ for ClusterConnectionPool and ClusterReadOnlyConnectionPool diff --git a/docs/Upgrading.md b/docs/Upgrading.md index 560ddd69..ce45a1a5 100644 --- a/docs/Upgrading.md +++ b/docs/Upgrading.md @@ -3,7 +3,7 @@ This document will describe what must be done when upgrading between different versions to ensure that code still works. -## 1.0.0 --> Next release +## 1.0.0 --> 1.1.0 The following exceptions have been changed/added and code that use this client might have to be updated to handle the new classes. diff --git a/rediscluster/__init__.py b/rediscluster/__init__.py index 6e8551d6..71a0d3d8 100644 --- a/rediscluster/__init__.py +++ b/rediscluster/__init__.py @@ -17,7 +17,7 @@ setattr(redis, "StrictClusterPipeline", StrictClusterPipeline) # Major, Minor, Fix version -__version__ = (1, 0, 0) +__version__ = (1, 1, 0) if sys.version_info[0:3] == (3, 4, 0): raise RuntimeError("CRITICAL: rediscluster do not work with python 3.4.0. Please use 3.4.1 or higher.") diff --git a/setup.py b/setup.py index 31c7c0df..8ed6f2b1 100644 --- a/setup.py +++ b/setup.py @@ -20,7 +20,7 @@ setup( name="redis-py-cluster", - version="1.0.0", + version="1.1.0", description="Cluster library for redis 3.0.0 built on top of redis-py lib", long_description=readme + '\n\n' + history, author="Johan Andersson", From 85d5ec6d7afbd52c57bfde488b45a77594fb81cb Mon Sep 17 00:00:00 2001 From: Grokzen Date: Tue, 27 Oct 2015 01:38:52 +0100 Subject: [PATCH 50/50] Add examples/basic.py script --- examples/basic.py | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 examples/basic.py diff --git a/examples/basic.py b/examples/basic.py new file mode 100644 index 00000000..4569a965 --- /dev/null +++ b/examples/basic.py @@ -0,0 +1,10 @@ +from rediscluster import StrictRedisCluster + +startup_nodes = [{"host": "127.0.0.1", "port": "7000"}] + +# Note: decode_responses must be set to True when used with python3 +rc = StrictRedisCluster(startup_nodes=startup_nodes, decode_responses=True) + +rc.set("foo", "bar") + +print(rc.get("foo"))