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

Replace memberlist service in favor of cortex provided service #2224

Merged
merged 5 commits into from
Jun 18, 2020

Conversation

periklis
Copy link
Collaborator

@periklis periklis commented Jun 16, 2020

What this PR does / why we need it:
This PR addresses the wiring work needed to use the non-experimental version of memberlist as provided by: cortexproject/cortex#2638

Which issue(s) this PR fixes:

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

cc @cyriltovena @pstibrany

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 16, 2020
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@periklis
Copy link
Collaborator Author

@pstibrany I believe it's time to give documentation also some love regarding memberlist. I can add an example configuration with a reference to a headless k8s service.

@pstibrany
Copy link
Member

@pstibrany I believe it's time to give documentation also some love regarding memberlist. I can add an example configuration with a reference to a headless k8s service.

That's a great idea. We have some documentation in Cortex, but nothing in Loki.

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2020

Codecov Report

Merging #2224 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2224      +/-   ##
==========================================
- Coverage   62.09%   62.08%   -0.01%     
==========================================
  Files         156      156              
  Lines       12531    12529       -2     
==========================================
- Hits         7781     7779       -2     
  Misses       4143     4143              
  Partials      607      607              
Impacted Files Coverage Δ
cmd/querytee/main.go 0.00% <0.00%> (ø)
pkg/loki/loki.go 0.00% <0.00%> (ø)
pkg/loki/modules.go 9.13% <0.00%> (+0.12%) ⬆️
pkg/querier/queryrange/downstreamer.go 95.87% <0.00%> (-2.07%) ⬇️

@periklis
Copy link
Collaborator Author

Hmm... i noticed that we don't expose the memberlist flags for loki. Maybe also a good time to do so. WDYT?

@pstibrany
Copy link
Member

Hmm... i noticed that we don't expose the memberlist flags for loki. Maybe also a good time to do so. WDYT?

Interesting, yes, we should do that. Thanks!

@periklis periklis changed the title Replace memberlist service in favor of cortex provided service [WIP] Replace memberlist service in favor of cortex provided service Jun 16, 2020
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Keeping an eye on this, looks good so far.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 17, 2020
@periklis periklis changed the title [WIP] Replace memberlist service in favor of cortex provided service Replace memberlist service in favor of cortex provided service Jun 17, 2020
@periklis
Copy link
Collaborator Author

@pstibrany @cyriltovena PTAL with the added docs and examples

Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Nice example. Thanks for all your work on this PR!

@cyriltovena cyriltovena merged commit 815c475 into grafana:master Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants