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

Consistent hashing v2 #477

Merged
merged 4 commits into from
Nov 19, 2021
Merged

Consistent hashing v2 #477

merged 4 commits into from
Nov 19, 2021

Conversation

Dieterbe
Copy link
Contributor

This is a rework of #336

It achieves the same thing (compatibility with carbon consistent hashing after graphite-project/carbon#196 was applied), but by creating a new route type (consistentHashing-v2 in addition to consistentHashing). This way existing users of consistentHashing will not see a sudden reshuffle of their metrics when upgrading carbon-relay-ng after this patch is merged.

I notice in the carbon project that lib/carbon/hashing.py has received many more changes over the years after that fix, so I'm very doubtful that the hashing behavior is now in sync with the current carbon version, hence I've marked this route type as "experimental" for now. I hope that someone with more carbon hashing expertise can help us clarify exactly which versions we are now compatible with and what we are still missing. (we miss replication support for example)

Towards the future, it would still be nice to implement more different CH route types that correspond accurately to certain carbon versions. Although even carbon CH has its limits too and I can see us implement a style that carbon doesn't have. (see #211)

@github-vincent-miszczak
@trinitronx
@hamelg
@loitho

Note that implementing it reuses the same route struct,
which causes some other complication and refactoring
@Dieterbe
Copy link
Contributor Author

Note the test I added should confirm that the new behavior differs from old behavior, and adds the missing endpoint.
This is not the case. Both hashrings are identical and miss the desired endpoint that was mentioned in #335
I even took #366 , and tried a similar test on it, and it also gives the same output (without the fix). maybe i'm missing something?
Here's the backported test (note, requires the path to be github.com/graphite-ng/carbon-relay-ng not github.com/grafana/carbon-relay-ng and run cd route && GO111MODULE=off go test -v -run=TestIssue335 .:

func TestIssue335(t *testing.T) {
	initialDestinations := []*destination.Destination{
		{Addr: "10.20.34.114:12003"},
		{Addr: "10.20.39.104:12003"},
		{Addr: "10.20.40.161:12003"},
		{Addr: "10.20.35.158:12003"},
		{Addr: "10.20.37.70:12003"},
		{Addr: "10.20.40.126:12003"},
		{Addr: "10.20.33.78:12003"},
		{Addr: "10.20.39.19:12003"},
		{Addr: "10.20.42.66:12003"},
		{Addr: "10.20.34.131:12003"},
		{Addr: "10.20.38.55:12003"},
		{Addr: "10.20.41.75:12003"},
		{Addr: "10.20.32.8:12003"},
		{Addr: "10.20.37.165:12003"},
	}
	hasherWithFix := NewConsistentHasherReplicaCount(initialDestinations, 2)
	fmt.Println("len of hashring", len(hasherWithFix.Ring))
	for _, v := range hasherWithFix.Ring {
		if v.Position == 59418 {
			fmt.Println("found our missing value!") // this should trigger
		}
		fmt.Printf("%+v\n", v)
	}
}

@Dieterbe
Copy link
Contributor Author

As mentioned before, i'm not quite sure this does the job, and i will need people who use this to report back. Anyway it's clearly marked as experimental, so let's just merge it.

@Dieterbe Dieterbe merged commit 37996a1 into master Nov 19, 2021
@Dieterbe Dieterbe deleted the consistentHashing-v2 branch November 19, 2021 01:11
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.

2 participants