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

Align carbon_ch with Python implementation #336

Conversation

github-vincent-miszczak
Copy link
Contributor

carbon-relay-ng has incorrect carbon_ch implementation.

It lacks graphite-project/carbon@024f9e6#diff-1486787206e06af358b8d935577e76f5 update

It fixes #335

@Dieterbe
Copy link
Contributor

Dieterbe commented Dec 3, 2018

for the record, the problem and fix in python are described at graphite-project/carbon#196

Copy link
Contributor

@robert-milan robert-milan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Before this can be merged unit tests need to be added to verify behavior before and after the fix, specifically focusing on the bug. Do you have time to write them?

route/consistent_hashing.go Show resolved Hide resolved
route/consistent_hashing.go Show resolved Hide resolved
@loitho
Copy link

loitho commented Oct 21, 2020

Hi there,
is it possible to merge this fix ? The issue with the hashing prevents us from using the Carbonate tool which is annoying as it's very useful.

Kind regards

@robert-milan
Copy link
Contributor

I don't currently have time to write the unit tests needed before merging this.

@Dieterbe
Copy link
Contributor

we shouldn't break carbon-relay-ng deployments when they upgrade.
so I think we should have a config flag to control the sharding logic (by default: backwards compat with carbon-relay-ng's previous behavior, and opt-in for the new "compatible with python relay" behavior), at some point we can do the breaking change of flipping the default when we do a major release. (for extra benefit, at that point, people will have had a chance to test the new behavior)

@Dieterbe Dieterbe mentioned this pull request Sep 16, 2021
@Dieterbe
Copy link
Contributor

#477 is the reworked version of this PR.

@Dieterbe
Copy link
Contributor

See #477 - thanks for the effort on this!

@Dieterbe Dieterbe closed this Nov 19, 2021
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

4 participants