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

fix: add Overrides to Flusher dependencies to prevent panic on startup #3151

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

rohankmr414
Copy link
Contributor

Signed-off-by: Rohan Kumar rohankmr414@gmail.com

What this PR does

This PR adds Overrides module as a dependency Flusher to prevent panics on startup. Before removing chunks storage in #755, Flusher depended on Store which depended on Overrides module, this might be the reason behind panics

Which issue(s) this PR fixes or relates to

Fixes #2866

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@rohankmr414 rohankmr414 requested a review from a team as a code owner October 6, 2022 16:45
@CLAassistant
Copy link

CLAassistant commented Oct 6, 2022

CLA assistant check
All committers have signed the CLA.

@rohankmr414 rohankmr414 changed the title 2866 fix: add Overrides to Flusher dependencies to prevent panic on startup Oct 6, 2022
@pracucci
Copy link
Collaborator

pracucci commented Oct 7, 2022

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

Copy link
Contributor

@osg-grafana osg-grafana left a comment

Choose a reason for hiding this comment

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

Looks good from a docs perspective; leaving the approval to an engineer

@pracucci
Copy link
Collaborator

pracucci commented Oct 7, 2022

The fix LGTM! Please address this to get it merged:
#3151 (comment)

Signed-off-by: Rohan Kumar <rohankmr414@gmail.com>
Signed-off-by: Rohan Kumar <rohankmr414@gmail.com>
Copy link
Collaborator

@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.

Thanks!

@pracucci pracucci enabled auto-merge (squash) October 7, 2022 10:11
@pstibrany
Copy link
Member

Do we know that this really fixes flusher? It would be nice to add integration test for flusher.

@pracucci pracucci merged commit c996c46 into grafana:main Oct 7, 2022
@rohankmr414 rohankmr414 deleted the 2866 branch October 7, 2022 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flusher: Panic on startup
5 participants