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 memberlist config from ring config. #3542

Merged
merged 1 commit into from
Mar 25, 2021
Merged

Remove memberlist config from ring config. #3542

merged 1 commit into from
Mar 25, 2021

Conversation

kavirajk
Copy link
Contributor

What this PR does / why we need it:
Remove memberlist_config from ingestor_config and distributor_config. Its invalid to provide memerlist_config on these configs.

Which issue(s) this PR fixes:
Fixes #2754

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@kavirajk kavirajk requested review from owen-d and a team March 25, 2021 18:31
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.

(This is a top level config, this is fixing a docs mistake)

@owen-d owen-d merged commit bcbffb1 into grafana:master Mar 25, 2021
@danpoltawski
Copy link
Contributor

Disappointed my pull request tackling the same problem a week ago couldn't be reviewed, was excited to contribute to the project #3505

@owen-d
Copy link
Member

owen-d commented Mar 25, 2021

Apologies and totally not our intention; we cleaned this up during the bug scrub today. We were looking at issue #2754 but it looks like there were a few issues related to this.

Anyways, we definitely appreciate and want to encourage external contributions and I'm sorry we failed you there. It's difficult for us to keep up will everything.

@danpoltawski
Copy link
Contributor

No worries, there is also #3498 which this partially fixes

@owen-d
Copy link
Member

owen-d commented Mar 25, 2021

Thanks for the heads up :). The Loki team is very small and some of us are being pulled in a few different directions these days. I rarely get to keep up with the PRs/issues like I used to. I expect that will improve again soon (at least in my case), but I can promise we're trying.

@owen-d
Copy link
Member

owen-d commented Mar 25, 2021

By the way, it looks like we missed this bit from your PR: https://github.com/grafana/loki/pull/3505/files#diff-025adfc5a8f641b9f5a1869996e3297b6c17f13933f52354cd9b375548ad7970R169-R171

Would you mind opening another with this change? I'll merge it for you!

@kavirajk
Copy link
Contributor Author

@danpoltawski apologies!. Also didn't notice that your PR is linked to this issue!. You are welcome to add the missing bit like @owen-d suggested :)

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.

Documentation: config for ingester/distributor memberlist is invalid
3 participants