Closed helielson closed 11 years ago
@Helielson we're probably lacking a bit on tests in this area
Ok @dcramer. Thanks!!!
Hi @dcramer.
After this pull request, I found a bug. These are the steps to reproduce the bug:
1- Start at least 2 redis servers.
2- Run some application that uses nydus.db.create_cluster method to handle the redis cluster. In my case, I started a simple flask application.
hosts = {
0 : {'host':'localhost', 'port':6290},
1 : {'host':'localhost', 'port':6291}
}
create_cluster({
'backend': 'nydus.db.backends.redis.Redis',
'router': 'nydus.db.routers.keyvalue.ConsistentHashingRouter',
'hosts': hosts
})
3- Restart the redis server.
After doing it, the mark_connection_up
method from nydus.db.routers.keyvalue.ConsistentHashingRouter
is called twice, one for each server that went up.
In the first call, the condition I added in this pull request returns None
as expected, because ketama nodes are empty (the servers were down).
What happens:
mark_connection_up
called with db_num
parameter as 0
(host key)if
condition of this pull request)self._hash.get_node(node_key) is None: #returns None, so add the node
self._hash.add_node(node_key)
self._hash.get_node(node_key)
is None, so, add node_key redis://localhost:6290/0
ConsistentHashingRouter._hash.nodes
is now set(['redis://localhost:6290/0'])
In the second call, the condition returns the first node added instead of returning None, as expected. I think it's a bug from get_node
method.
# node_key is `redis://localhost:6291/0`
self._hash.get_node(node_key) is None: # returns redis://localhost:6290/0
self._hash.add_node(node_key)
Is there a bug with get_node
method?
If no, please, revert this pull request.
If you need more information about this bug, let me know.
Thanks in advance.
I did this change for performance improvement. I think it's not necessary to add the node key to hash if the node key was already added. After this change, the redis access time decreased 75%. @dcramer, I didn't found any test case where the node key was None. What's the better way to test it?