-
Notifications
You must be signed in to change notification settings - Fork 21
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
bucky: first stab at supporting fnv1a_ch hash algo #18
Conversation
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'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:
|
That would actually be less guesswork for the parsing, I think. I struggled a bit with it, as you probably have seen :) |
t.nodes = append(t.nodes, node) | ||
for i := 0; i < t.replicas; i++ { | ||
var e RingEntry | ||
replica_key := fmt.Sprintf("%s:%d", node.KeyValue(), i) | ||
e.position = computeRingPosition(replica_key) | ||
replica_key := fmt.Sprintf("%d-%s", node.FNV1aKeyValue(), i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is backward here. It looks like the format string should be "%s-%d"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this looks wrong indeed.
|
I can produce the hash-ring position dump easily, what kind of tests do you have in mind? |
I've updated 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. |
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 |
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. |
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.