Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed long cachekeys reported on issue #380 #385

Merged
merged 1 commit into from Aug 11, 2014

Conversation

trein
Copy link
Contributor

@trein trein commented Jun 25, 2014

No description provided.

@trein
Copy link
Contributor Author

trein commented Jun 25, 2014

Ok. Locally all tests are passing, but they fail on Travis. Do you have any ideas?

@hanshasselberg
Copy link
Member

Ruby 1.8.7 doesn't have the method base64digest: http://www.ruby-doc.org/stdlib-1.8.7/libdoc/digest/rdoc/Digest/Class.html.

@trein
Copy link
Contributor Author

trein commented Jun 25, 2014

Good catch! I haven't noticed you guys are using Ruby 1.8.7. I will propose a new way to hash the cache_key then.

@Zapotek
Copy link
Contributor

Zapotek commented Jun 25, 2014

Zlib.crc32 worked for me, fast and small, and zlib is already required by that file.

@Zapotek
Copy link
Contributor

Zapotek commented Jun 25, 2014

@i0rek Just saw your remark on CRC32 as a hashing function on #380, is it about the collisions?
Also, unless you're worried about persistency, you can just use #hash and force it #to_s.

If none of these are good enough, you can use MurmurHash (which is what Ruby uses for strings) which is again fast and small.

@hanshasselberg
Copy link
Member

@Zapotek
Copy link
Contributor

Zapotek commented Jun 25, 2014

Hm, fair enough. Better safe than sorry then, there's also a MurmurHash3 implementation available as a gem which is faster than MurmurHash 1 or 2: https://github.com/funny-falcon/murmurhash3-ruby

I think I'll switch my project to MurmurHash3 too, heh. Thanks for the info man.

@hanshasselberg
Copy link
Member

at least for Typhoeus I would prefer a stdlib solution...

@trein
Copy link
Contributor Author

trein commented Jul 13, 2014

Hi guys.

Sorry for the delay. I've switched the hashing method from SHA1.base64digest to SHA1.hexdigest, which is available on ruby 1.8.7 stdlib. Let me know your thoughts.

@hanshasselberg
Copy link
Member

Great! Since cache_key and hash are very similar now, could you alias one to another?
Btw I looked up the issue where the long cache keys were discussed: #345. The reason it was done that way is that Dalli can deal with long cache keys. Since other libs don't do that I think it is appropriate to switch to SHA1.

@trein
Copy link
Contributor Author

trein commented Aug 7, 2014

Hi @i0rek,

Probably I haven't understood properly, but I'm currently calling cache_key from hash. Is that what you meant by alias?

def hash
    Zlib.crc32 cache_key
end

The only difference is that I'm encoding it using CRC as the return shouldn't be a string.

@hanshasselberg
Copy link
Member

@trein I am sorry. That was confusing. Thank you for your work. I will merge and make the tiny change myself. Should have done that in the first place instead of bothering you.

hanshasselberg added a commit that referenced this pull request Aug 11, 2014
fixed long cachekeys reported on issue #380
@hanshasselberg hanshasselberg merged commit 06314ee into typhoeus:master Aug 11, 2014
@hanshasselberg
Copy link
Member

I was talking without thinking. hash returns an int...
Thanks again for your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants