search

joaojeronimo / node_redis_cluster

A thin wrapper over node_redis to make it work with Redis Cluster
53 stars 17 forks source link

[*] changed algorithm for hashing to native redis crc16 CCIT Xmodem #10

Open h0x91b opened 9 years ago

joaojeronimo commented 9 years ago

Not cool, doing the V8 - Native ... Native - V8 rountrips makes computing the crc16 significantly slower than computing it in JS, bringing native code in this module will make it impossible to use in environments where people do not have build tools (which is a lot), and if the crc16 algorithm has a problem there must be a fix we can apply to it, replacing it for a native C crc16 doesn't seem a good idea.

h0x91b commented 9 years ago

There is no solution in JS for utf-8 multibyte crc16...

h0x91b commented 9 years ago

Try this test:

assert.equal(crc.crc16(new Buffer('123456789')).toString(16), '31c3'); assert.equal(crc.crc16(new Buffer('привет')) % 16384, 7365); assert.equal(crc.crc16(new Buffer('nht.reach.accounts:زووم')) % 16384, 4107);

I checked 5 JS libraries and all checked libs doing it wrong...

check issue #9

joaojeronimo commented 9 years ago

I understand that, still don't think it justifies bringing a native module to the dependencies. Options are 1) fix one of the 5 libraries that don't work 2) accept this as a caveat and tell people to use ASCII keys, shouldn't be too hard.

h0x91b commented 9 years ago

Ok, I'll try to fix it

 

Arseniy Pavlenko | CTO

M. +972-54-5417509

scrnz.com

On Fri, Jan 23, 2015 at 1:32 PM, João Jerónimo notifications@github.com wrote:

I understand that, still don't think it justifies bringing a native module to the dependencies. Options are 1) fix one of the 5 libraries that don't work 2) accept this as a caveat and tell people to use ASCII keys, shouldn't be too hard.

Reply to this email directly or view it on GitHub: https://github.com/joaojeronimo/node_redis_cluster/pull/10#issuecomment-71181388