joaojeronimo / node_redis_cluster

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

Error when hashing the slot for unicode strings #9

Closed danmaz74 closed 9 years ago

danmaz74 commented 9 years ago

When hashing for example this string:

nht.reach.accounts:زووم

the slot computed is 15073, while according to redis the slot is 4107

This causes an Error: MOVED 4107 ... that cannot be fixed.

h0x91b commented 9 years ago

yeah, I see.

➜ redis-tool git:(master) ✗ redis-cli -c -p 7002 127.0.0.1:7002> set nht.reach.accounts:زووم 123 -> Redirected to slot [4107] located at 127.0.0.1:7001 OK

Thanks, I will fix it.

danmaz74 commented 9 years ago

Thanks @h0x91b that's great - let me know if I can help in any way in your research/bugfixing. This is very urgent for me :)

For example I can find other instances of the problem/create some test cases.

h0x91b commented 9 years ago

Looks like there is no JS implementations which do it right...

I`ll try to implement it in C, this can take some time..

h0x91b commented 9 years ago

Fixed in branch hash-slots-utf8

Fixed by writing native C module for hashing https://github.com/h0x91b/node-redis-crc16

Check it please

danmaz74 commented 9 years ago

Thx. I installed the new npm with npm install node-redis-crc16 --save

The compilation looks ok:

CXX(target) Release/obj.target/crc-itu/lib/crc-itu.o SOLINK_MODULE(target) Release/obj.target/crc-itu.node SOLINK_MODULE(target) Release/obj.target/crc-itu.node: Finished COPY Release/crc-itu.node

Then I changed my code like this:

redisCluster = require('node-redis-crc16').clusterClient

But when I launched I got:

Error: /opt/cybranding-dan/node_modules/node-redis-crc16/build/Release/crc-itu.node: undefined symbol: init at Object.Module._extensions..node (module.js:485:11) at Module.load (/usr/local/lib/node_modules/coffee-script/lib/coffee-script/register.js:45:36) at Function.Module._load (module.js:312:12) at Module.require (module.js:362:17) at require (module.js:378:17) at Object. (/opt/cybranding-dan/node_modules/node-redis-crc16/index.js:1:79) at Module._compile (module.js:449:26) at Object.Module._extensions..js (module.js:467:10) at Module.load (/usr/local/lib/node_modules/coffee-script/lib/coffee-script/register.js:45:36) at Function.Module._load (module.js:312:12) at Module.require (module.js:362:17) at require (module.js:378:17) at Object. (/opt/cybranding-dan/nodejs/hashtagify/data-server.coffee:935:16) at Object. (/opt/cybranding-dan/nodejs/hashtagify/data-server.coffee:1:1) at Module._compile (module.js:449:26)

Any ideas?

h0x91b commented 9 years ago

No, node-redis-crc16 is deferent module which I made.

You need to change your package.json:

"dependencies": { "redis-cluster": "git+https://github.com/joaojeronimo/node_redis_cluster.git#hash-slots-utf8" },

and run npm install

h0x91b commented 9 years ago

Just change package.json run npm install and use a redis-cluster as usual...

danmaz74 commented 9 years ago

Ok thanks, but I still get the same error:

Error: /opt/cybranding-dan/nodejs/node_modules/redis-cluster/node_modules/node-redis-crc16/build/Release/crc-itu.node: undefined symbol: init

A little googling suggests that the init function should be declared as extern "C"

But it's weird if you were able to install and test your code. Anyway I'll try this change as soon as I can.

danmaz74 commented 9 years ago

I confirm - the extern "C" fixed the bug

I'll make a pull request

And thanks a lot

h0x91b commented 9 years ago

Cool, have a nice day

danmaz74 commented 9 years ago

You too