Skip to content

memberlist: Use advertised address when sending packets #1857

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

Merged
merged 5 commits into from
Dec 3, 2019
Merged

memberlist: Use advertised address when sending packets #1857

merged 5 commits into from
Dec 3, 2019

Conversation

pstibrany
Copy link
Contributor

Change TCP transport to use advertised address when sending "packets" (term used by memberlist lib). This was needed to stop memberlist complaining about not being able to ping other members of the cluster when running on internal Grafana k8s cluster.

(Memberlist is still experimental)

@bboreham
Copy link
Contributor

Seems kinda nuts that we need a 600-line file just to add a transport to use a library.

I read your comments about mesh in the design doc; those 'todo's were written 2-3 years ago and it's extremely unlikely it will change except to fix bugs.

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I'm missing some knowledge about how how it works under the hood, but the changes in this PR look OK to me. I just left a tiny comment about commented code.

@pstibrany
Copy link
Contributor Author

Seems kinda nuts that we need a 600-line file just to add a transport to use a library.

It would be nice if TCP-based transport was part of memberlist. There are some comments here and there indicating that people are using it with TCP transport, but nothing is included in the lib itself.

I read your comments about mesh in the design doc; those 'todo's were written 2-3 years ago and it's extremely unlikely it will change except to fix bugs.

If you want to encourage more people to use it (and maybe you don't, which is fine), it may be worthwhile to remove TODO's from the code, clean up the API little bit and add some documentation to it.

Signed-off-by: Peter Štibraný <[email protected]>
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

@gouthamve gouthamve merged commit 150f2ae into cortexproject:master Dec 3, 2019
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.

5 participants