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

auto-apply memberlist ring config when join_members provided #4400

Merged
merged 4 commits into from
Oct 7, 2021

Conversation

trevorwhitney
Copy link
Collaborator

@trevorwhitney trevorwhitney commented Sep 30, 2021

Signed-off-by: Trevor Whitney [email protected]

What this PR does / why we need it:

This PR will change the default ingester, distributor, and ruler ring configurations to use memberlist if -memberlist.join_members in provided. The idea here is that if a user explicitly configured the memberlist configuration section, they probably want to be using memberlist for their various ring configurations. Since a user can still explicitly override a specific ring configuration (for example, use consul for the distributor), it seems pretty harmless to take a guess at better defaults here.

Checklist

  • Documentation added
  • Tests updated

@trevorwhitney trevorwhitney requested a review from a team as a code owner September 30, 2021 21:57
Base automatically changed from loki-common-config to main October 1, 2021 18:13
@pull-request-size pull-request-size bot added size/XL and removed size/M labels Oct 1, 2021
@pull-request-size pull-request-size bot added size/M and removed size/XL labels Oct 1, 2021
@trevorwhitney trevorwhitney force-pushed the populate-memberlist branch 2 times, most recently from 8bc0f45 to a801881 Compare October 1, 2021 22:23
Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

Looks very reasonable to me 👍 .

Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM

@slim-bean
Copy link
Collaborator

@trevorwhitney my only nit would be to include a brief comment with basically what you summarized in the PR

As this common config block grows it's gonna be a tangled web of us manipulating config and any insight I think will go a long away to help our furture selves.

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.

I believe this is showing some diffs from a prior PR (compactor prefix in shared config), which will probably go away after merging in main. That being said, LGTM

Signed-off-by: Trevor Whitney <[email protected]>
@trevorwhitney
Copy link
Collaborator Author

@owen-d docs added. It seemed from previous PRs that the configuration docs are updated by hand (which I did), but if they're auto-generated from somewhere else please let me know and I'll update the source.

@owen-d owen-d merged commit b07920c into main Oct 7, 2021
@owen-d owen-d deleted the populate-memberlist branch October 7, 2021 22:06
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.

4 participants