twitter / twemproxy

A fast, light-weight proxy for memcached and redis
Apache License 2.0
12.14k stars 2.06k forks source link

Error response is not forward to client on redis mulit del command #216

Open idning opened 10 years ago

idning commented 10 years ago

if one of the redis server is read only slave:

del key1 will got Err:

READONLY You can't write against a read only slave.

del key1 key2 key3 will got Err:

Invalid argument

here is the test case:

def test_multi_delete_on_readonly():
    all_redis = [
            RedisServer(USER, '127.0.0.5:2100', '/tmp/r/redis-2100/'),
            RedisServer(USER, '127.0.0.5:2101', '/tmp/r/redis-2101/'),
        ]
    for r in all_redis:
        r.args['cluster_name'] = 'ttt'
        r.args['server_name'] = TT('redis-$port', r.args)
        r.deploy()
        r.start()

    nc = NutCracker(USER, '127.0.0.5:4100', '/tmp/r/nutcracker-4100/', all_redis)
    nc.args['cluster_name'] = 'ttt'
    nc.deploy()
    nc.start()

    all_redis[0].slaveof(all_redis[1].args['host'], all_redis[1].args['port'])
    r = redis.Redis('127.0.0.5', 4100)
    keys = ['key-1', 'key-2', 'kkk-3']

    r.delete('key-1') #got READONLY You can't write against a read only slave.
    r.delete('key-2')
    r.delete('key-3')
    print r.delete(*keys) #got Invalid argument

here is the code make this different:

https://github.com/twitter/twemproxy/blob/master/src/proto/nc_redis.c#L2063-L2074

idning commented 10 years ago

the test case is here: https://github.com/idning/test-twemproxy/blob/master/tests/test_del.py#L59-L69

manjuraj commented 9 years ago

@idning is this still an issue?

idning commented 9 years ago

@manjuraj when we want to do a multi-del on many keys, and one of the redis-servers is READONLY.

some of the keys will be deleted success. but what we return to client is "Invalid argument", this is not good.

I think return READONLY is somehow better. so this is still a problem we need to solve.