search

jjneely / buckytools

Go implementation of useful tools for dealing with Graphite's Whisper DBs and Carbon hashing
Other
87 stars 21 forks source link

bucky: first stab at supporting fnv1a_ch hash algo #18

Closed grobian closed 7 years ago

grobian commented 7 years ago

Support fnv1a_ch next to carbon_ch. This means port needs to be recognised, which can be given in the form HOST[:PORT[:INSTANCE]]. The previous form of HOST[:INSTANCE] is still supported by parsing instance as a number, if so, it is assumed to be a port.

I would like some basic review on the sanity of this change. I can't imagine everything works as expected this way.

jjneely commented 7 years ago

I've stumbled over my own design flaw of SERVER:PORT or SERVER:INSTANCE before. So that is the crux of the issue here. I need to fix that and do what you do with carbon-c-relay:

host[:port][=instance]
grobian commented 7 years ago

That would actually be less guesswork for the parsing, I think. I struggled a bit with it, as you probably have seen :)

jjneely commented 7 years ago

grobian-fnv1a_ch is my WIP branch with your code, Grobian. Do you have any test data that I can use with this that is verified? Building some tests to make sure the fnv1a code works as expected is next.

grobian commented 7 years ago

I can produce the hash-ring position dump easily, what kind of tests do you have in mind?

jjneely commented 7 years ago

I've updated grobian-fnv1a_ch with a fnv1a_test.go file with some basic sanity tests, nothing exhaustive. At this point, my basic tests that the hashing algorithm produces sane results is not matching the results I get from running carbon-c-relay in test mode. Can you take a look at that?

Once things are working, I'd like to have some more tests as I've definitely had strange corner cases in the hashing algorithms come up in the past.

grobian commented 7 years ago

I think we can use something like https://github.com/grobian/carbon-c-relay/blob/master/test/issue236.out as base here, I'll extract the first hits for all of the inputs

jjneely commented 7 years ago

grobian-fnv1a_ch updated again. This looks like it works and passes the limited tests I've setup. More tests are double plus good here.

I'm not sure what was going on with the Golang standard library version of the FNV1a32 hash, but it was giving me very odd results. Being that I implemented FNV1a64 internally for the jump hash, I did so for FNV1a32 and she started working like a charm.

There are changes in buckyd in this code base that's have not been reflected in the bucky client. That's forth coming.