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

Remove gossip #1008

Merged
merged 9 commits into from
May 28, 2019
Merged

Remove gossip #1008

merged 9 commits into from
May 28, 2019

Conversation

povilasv
Copy link
Member

@povilasv povilasv commented Apr 4, 2019

Changes

Remove gossip 🔥

Issue

#734

Verification

Tests pass

Tested this in my k8s env, couldn't find a issue

@povilasv povilasv marked this pull request as ready for review May 24, 2019 15:58
@povilasv
Copy link
Member Author

povilasv commented May 24, 2019

I plan to test this in dev on monday, Other than that, it's ready for review :)

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM, cannot find any blocker (: LGTM 🔥

test/e2e/spinup_test.go Outdated Show resolved Hide resolved
@povilasv
Copy link
Member Author

tested a bunch of components in my dev k8s cluster, it looks like it works :D

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

This LGTM however we still have some references to gossip:

  • docs/component/{query,rule,sidecar,store}.md: from other components if you use gossip, I guess this part can just be removed. It comes from cmd/thanos/flags.go.
  • pkg/query/storeset.go has a log message talking about gossip: duplicated address in gossip or static store nodes -> duplicated address detected. There's also a comment there talking about gossip. We should nuke it.
  • pkg/store/prometheus.go also has a small comment about gossip meta. We should remove it 💣

@GiedriusS
Copy link
Member

I guess it's not part of it but we should also remove gossip info from the example Grafana dashboards and the benchmark/cmd/thanosbench/*. Care to create an issue for that? It's not a blocker 😛

@povilasv
Copy link
Member Author

@GiedriusS

thanks

PTAL

@povilasv
Copy link
Member Author

Issue: #1180

@povilasv povilasv requested a review from GiedriusS May 28, 2019 12:11
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

@brancz
Copy link
Member

brancz commented May 28, 2019

Love it! Let's get rid of it finally!

@brancz brancz merged commit 37f89ad into thanos-io:master May 28, 2019
@povilasv povilasv deleted the remove-gossip branch May 29, 2019 07:07
@povilasv
Copy link
Member Author

🎉

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.

4 participants