arjunmehta / node-georedis

Super fast geo queries.
MIT License
192 stars 33 forks source link

Alternative to using MULTI/EXEC #4

Closed nidhhoggr closed 8 years ago

nidhhoggr commented 8 years ago

Hello and thanks for the great repo. Can you briefly explain the reason for using MULTI/EXEC? I am scaling redis using Twemproxy and unfortunately transaction operations are not supported. With that being said is there a way to implement emulation on a distributed redis cluster? Take note that twemproxy can route requests to appropriate shard via hash_tag. Twemproxy can also route requests by port to a sole redis instance provided in nutcracker configuration.

arjunmehta commented 8 years ago

@zmijevik Thanks for this! And for your PR.

To answer your question about why I used MULTI/EXEC... it was the easiest implementation at the time to stack multiple queries in one request. As you saw in the dependencies I was using a fairly earlier version of node-redis.

I didn't know about .batch() and I wonder if the choice between multi and batch is even needed. It seems like batch is the more performant option and atomic transactions are not essential at all since this is a read operation... As long as errors are caught if any one of the queries fails which batch seems to support.

Do you think it would make sense to just remove .multi() and always use .batch()? Do some older versions of Redis Server not support it or does node-redis account for that?

arjunmehta commented 8 years ago

So instead of

if(options.transactionsUnsupported !== true) {
    var multi = client.multi();
} else {
    var multi = client.batch();
}

do

var multi;
if(typeof client.batch === 'function') {
    multi = client.batch();
} else {
    multi = client.multi();
}

Ideally the check wouldn't even need to happen on every query.

nidhhoggr commented 8 years ago

I think that is perfect. This would omit the need to specify the transactionsUnsupported option. Regarding batch support for older instances of Redis, I don't think that would be an issues because it seems that the only notable differences between exec_transaction and exec_batch is that exec_transaction send Redis the Multi and Batch command, whereas exec_batch just sends individual commands and queues them to a response array. Let me know if you can apply the code above or if you would like me to revise the PR. Thanks, looking forward to getting this working with Twemproxy.

arjunmehta commented 8 years ago

Great! Yes please modify the PR with the changes I commented and I'll merge it in, version bump and publish. Thanks so much!