disqus / nydus

Nydus is a Python toolkit for managing database connections and routing operations, primarily for Redis
Apache License 2.0
380 stars 38 forks source link

Bug in the get_node method #28

Closed helielson closed 11 years ago

helielson commented 11 years ago

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:

self._hash.get_node(node_key) is None: #returns None, so add the node
    self._hash.add_node(node_key)

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.

dcramer commented 11 years ago

@Helielson I will revert this for now, and we can revisit addressing the issue. It does sound like there might be a bug though.