Add watch, unwatch, mul method to pipeline #201
Conversation
max8899
commented
Jun 21, 2017
max8899
commented
Use transaction with pipeline in redis cluster Do the operation with keys with same prefix, so that command will be executed on the same node * pipeline Signed-off-by: Lei Gong <xue177125184@gmail.com>
Use transaction with pipeline in redis cluster Do the operation with keys with same prefix, so that command will be executed on the same node * Reference pipeline function Signed-off-by: Lei Gong <xue177125184@gmail.com>
|
@Grokzen When could my PR be reviewed? It kind of block my features which being reviewed in other projects. |
|
@max8899 Adding these methods and specially to the pipeline support requires detailed review before i will merge this. Adding these methods might not be as easy as it looks. One thing that i would require before mering is tests of this functionality. There should be tests inside Will review this sometime late next week. |
| @@ -162,6 +163,10 @@ def send_cluster_commands(self, stack, raise_on_error=True, allow_redirections=T | |||
|
|
|||
| # 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. | |||
|
|
|||
| if c.args[0] in ['MULTI', 'UNWATCH']: | |||
There was a problem hiding this comment.
The comments that is above this belongs to the line below node_name = node['name']. Move the comments down or move this new if case up above the comments.
There was a problem hiding this comment.
Should commands on a single node or all commands be unwatched if exception raised?
| @@ -34,7 +35,7 @@ def __init__(self, connection_pool, result_callbacks=None, | |||
| self.startup_nodes = startup_nodes if startup_nodes else [] | |||
| self.nodes_flags = self.__class__.NODES_FLAGS.copy() | |||
| self.response_callbacks = dict_merge(response_callbacks or self.__class__.RESPONSE_CALLBACKS.copy(), | |||
| self.CLUSTER_COMMANDS_RESPONSE_CALLBACKS) | |||
| self.CLUSTER_COMMANDS_RESPONSE_CALLBACKS, {'DEL': parse_del}) | |||
There was a problem hiding this comment.
Why do you need this? I do not see a reason for it elsewhere in the code.
| """ | ||
| """ | ||
| raise RedisClusterException("method unwatch() is not implemented") | ||
| if not names: | ||
| raise RedisClusterException("UNWATCH command needs at least on name in this pipeline") |
There was a problem hiding this comment.
Possibly change the error message to UNWATCH command needs at least one name when used in a pipeline
| @@ -262,12 +276,18 @@ def load_scripts(self): | |||
| def watch(self, *names): | |||
| """ | |||
| """ | |||
| raise RedisClusterException("method watch() is not implemented") | |||
| if not names: | |||
| raise RedisClusterException("WATCH command needs at least on name in this pipeline") | |||
There was a problem hiding this comment.
Possibly change the error message to WATCH command needs at least one name when used in a pipeline
|
|
||
|
|
||
| def parse_del(response, *args, **kwargs): | ||
|
|
There was a problem hiding this comment.
No blank line here. Remove blank line or add empty docstring
| self.connection_pool._available_connections[node_name].remove(n.connection) | ||
|
|
||
| if self.connection_pool._created_connections_per_node.get(node_name, 0): | ||
| self.connection_pool._created_connections_per_node[node_name] -= 1 |
There was a problem hiding this comment.
I am not sure, but should a connection be discarded directly? The connection pool then seems to be meaningless
|
@max8899 Bump. This MR still has open review items that needs to be handled in order for this MR to be reviewd again and in the future merged. |
|
@max8899 Bump on this PR, i still have review comments above that is not dealt with. I will probably not merge this into the next release but the one after that if comments is looked on and fixed. |
|
Need |
|
Closing this issue as it is not active any longer and the work is to far behind to be implemented right now. A new set of work for this feature would have to be done. |
