forward-port CRC32 code from old version #13

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

bengl commented Feb 26, 2013

For a variety of reasons, the code I'm working on is dependent on an old version of node-memcached. Unfortunately, in versions prior to 0.1.1, node-memcached's package.json does not specify a version of hashring. In the default configuration in these older versions, node-memcached uses 'crc32' as it's hashing algorithm option. Unfortunately, support for this seems to have been dropped in hashring v1.0.0. Now we're in a state where new installs of any version of node-memcached prior to 0.1.1 are broken.

For example, instantiating a Memcache object (at v0.0.7) using "localhost:11211" as a parameter causes the following:

crypto.js:132
  return new Hash(hash);
         ^
Error: Digest method not supported
    at Object.exports.createHash (crypto.js:132:10)
    at HashRing.hash (/Users/bengl/memcachedtest/node_modules/memcached/node_modules/hashring/index.js:199:17)
    at HashRing.digest (/Users/bengl/memcachedtest/node_modules/memcached/node_modules/hashring/index.js:210:15)
    at each (/Users/bengl/memcachedtest/node_modules/memcached/node_modules/hashring/index.js:112:16)
    at Array.forEach (native)
    at HashRing.generate [as continuum] (/Users/bengl/memcachedtest/node_modules/memcached/node_modules/hashring/index.js:100:11)
    at new HashRing (/Users/bengl/memcachedtest/node_modules/memcached/node_modules/hashring/index.js:77:8)
    at EventEmitter.Client (/Users/bengl/memcachedtest/node_modules/memcached/lib/memcached.js:68:19)
    at Object. (/Users/bengl/memcachedtest/index.js:2:10)
    at Module._compile (module.js:449:26)

We dealt with this internally by passing around a tarball of our app with the versions of everything frozen, but we'd like to be able to depend on npm instead.

In the spirit of ensuring older versions are not broken forever, I see two options:

  1. Do a force publish on the older versions of node-memcached to npm, adding a version of 0.0.x for hashring to the package.json.
  2. Re-implement the missing hash algorithm in the new code.

Option 1 might be the easiest, and that way you could worry a little less about backward compatibility issues. I'm generally a little timid about doing a force publish, but in this case I think it might be worth it, since the change would be so small and the fix would be tremendously helpful. I'd be perfectly happy to blow away my node_modules directory if it means this problem goes away. :)

Since option 2 may or may not be safer, I've gone ahead and done just that in this pull-request, in case you want to go down that route instead.

Owner

3rd-Eden commented Feb 26, 2013

Hey,

I did not know that this impacted older versions of my memcached driver. So that is definitely an issue here. I could forcefully publish an update to those packages with a fixed version number.

The reason that I removed my crc32 hashing is because it's implementation was wrong and did not fully work with unicode and did some weird hacks to transform a numeric output to a string so I could rehash it.. I must have been pretty high when I wrote that shit. So that was clearly a mistake and not something I want to make again in this point release.

I have been planning to add support for hashing algorithms that return a numeric value like murmur and fnv hashing. But I haven't been able to add that yet without completely destroying the current implementation.

bengl commented Feb 26, 2013

Sure, awesome! A force-publish would definitely solve this. 😃 For reference, the version of node-memcached we're using that led me to this is 0.0.7. I guess this can be closed once everything's published to npm?

Owner

3rd-Eden commented Feb 26, 2013

Yes, I think that a force push would be the best way to deal with this and keep this issue open to properly implement the CRC32 algorithm. I'll give you a heads up on when this has been done.

Owner

3rd-Eden commented Feb 26, 2013

Force pushed 0.0.7, will re-publish everything from 0.0.6 to 0.1.0

bengl commented Feb 26, 2013

Thank you!

@3rd-Eden 3rd-Eden closed this Aug 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment