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

Is there a plans to merge jump branch to the master? #8

Closed
azhiltsov opened this issue Oct 31, 2016 · 15 comments
Closed

Is there a plans to merge jump branch to the master? #8

azhiltsov opened this issue Oct 31, 2016 · 15 comments

Comments

@azhiltsov
Copy link

No description provided.

@jjneely
Copy link
Owner

jjneely commented Nov 2, 2016

Are you looking to use that with carbon-c-relay's jump-fnv1a hashing algorithm? IIRC, that's what I was trying to imitate here. But being that I've never been able to push a hashring change in my current cluster this hasn't been merged.

@azhiltsov
Copy link
Author

Yes. We are indeed trying to use it with carbon-c-relay's jump-fnv1a as it gives us more equal distribution of metrics between hosts.
We currently switching our setup to a replication factor 1, so the #3 is not a case for us any more.

@jjneely
Copy link
Owner

jjneely commented Nov 3, 2016

The fact that Graphite just does multiple destinations rather than true replication has been a big reason why I've not done more work on replication factor > 1. There's no real redundancy / data safety that adds.

I'll take a look at getting that merged in.

@deniszh
Copy link
Contributor

deniszh commented Feb 5, 2017

Hello!
Does someone have working jumphash implementation for buckytools? @azhiltsov ? @jjneely ?

@azhiltsov
Copy link
Author

Hi. Yes, we currently using it.
It kinda hidden, we accidentally found it here https://github.com/jjneely/buckytools/tree/jump

@deniszh
Copy link
Contributor

deniszh commented Feb 5, 2017

Ah, so you using a build from jump tree and it works, right? Cool, thanks!

@deniszh
Copy link
Contributor

deniszh commented Feb 5, 2017

Nah, can't build it:
"./cluster.go:67: master.Algo undefined (type *buckytools.JSONRingType has no field or method Algo)
./cluster.go:69: undefined: hashing.NewCarbonHashRing
./cluster.go:71: undefined: hashing.NewJumpHashRing
./cluster.go:71: master.Replicas undefined (type *buckytools.JSONRingType has no field or method Replicas)
./cluster.go:73: master.Algo undefined (type *buckytools.JSONRingType has no field or method Algo)
./cluster.go:74: master.Algo undefined (type *buckytools.JSONRingType has no field or method Algo)
./cluster.go:118: master.Algo undefined (type *buckytools.JSONRingType has no field or method Algo)
./cluster.go:118: v.Algo undefined (type *buckytools.JSONRingType has no field or method Algo)
make: *** [bucky] Error 2"

@deniszh
Copy link
Contributor

deniszh commented Feb 5, 2017

Ah, of course - all imports are pointed to master version. So, the only way to build it - fork into the own repo, rename jump branch to master and change imports. Meh. Will test it and will try to properly merge it to master.

@jjneely
Copy link
Owner

jjneely commented Feb 21, 2017

This is in progress on my end....

@jjneely
Copy link
Owner

jjneely commented Feb 23, 2017

This is merged. I'm sure there are a bug or three that I've forgotten about.

@jjneely jjneely closed this as completed Feb 23, 2017
@howdoicomputer
Copy link

howdoicomputer commented Mar 3, 2017

@jjneely I think this breaks the 'standard' carbon hash implementation because the isHealthy function is checking to see if the hash ring contains any replicas.

@jjneely
Copy link
Owner

jjneely commented Mar 7, 2017

I'm sure there are some bugs here... Do have an error case to reproduce?

I plan to be spending some more time on this in the coming weeks, I just unblocked myself from another issue this evening.

@howdoicomputer
Copy link

@jjneely So I could be totally misunderstanding how everything works but this is what I did:

master...howdoicomputer:master

@deniszh
Copy link
Contributor

deniszh commented Mar 7, 2017

I did an exactly same thing in my branch - deniszh/buckytools@ad89e74 - remove '+1' in ring length comparison. Still not understand how it should work with it.

@jjneely
Copy link
Owner

jjneely commented Mar 7, 2017

Good to know....

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

No branches or pull requests

4 participants