node_redis_cluster icon indicating copy to clipboard operation
node_redis_cluster copied to clipboard

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

Open h0x91b opened this issue 10 years ago • 5 comments

h0x91b avatar Jan 22 '15 11:01 h0x91b

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.

joaojeronimo avatar Jan 22 '15 17:01 joaojeronimo

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

h0x91b avatar Jan 22 '15 17:01 h0x91b

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

h0x91b avatar Jan 22 '15 17:01 h0x91b

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.

joaojeronimo avatar Jan 23 '15 11:01 joaojeronimo

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 [email protected] 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

h0x91b avatar Jan 23 '15 11:01 h0x91b