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

Added local backend support to new ruler storage config #3932

Merged

Conversation

pracucci
Copy link
Contributor

@pracucci pracucci commented Mar 10, 2021

What this PR does:
In the PR #3805 Jacob has introduced the new ruler storage config using the Thanos objstore clients. As discussed several Cortex community meetings ago, we would like to deprecate the old objstore clients/config and to do that we need to add local backend support to the new config, which is what I'm proposing in this PR.

To avoid cyclic dependencies I had to do a bit of refactoring:

  • Moved RuleGroupList to pkg/ruler/rulespb (this is something we've done in other packages too and I think it's fine)
  • Move errors to /pkg/ruler/rulestore/errors (not a big fan of this, suggestions are welcome)

Which issue(s) this PR fixes:
N/A

Checklist

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

@pracucci pracucci requested a review from jtlisi March 10, 2021 12:58
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

Move errors to /pkg/ruler/rulestore/errors (not a big fan of this, suggestions are welcome)

I also am not a fan of this. However, the only alternative I can think of is to move the config struct for the rulestore to some kind of factory package. I imagine pkg/ruler/storage.go would move there as well. I feel this is equally as distasteful as the current approach.

@pracucci pracucci force-pushed the add-local-support-to-new-ruler-config branch from cb7332c to 0a91db9 Compare March 12, 2021 08:06
@pracucci
Copy link
Contributor Author

Move errors to /pkg/ruler/rulestore/errors (not a big fan of this, suggestions are welcome)

I also am not a fan of this. However, the only alternative I can think of is to move the config struct for the rulestore to some kind of factory package. I imagine pkg/ruler/storage.go would move there as well. I feel this is equally as distasteful as the current approach.

As @pstibrany pointed out offline to me, it wasn't needed anymore after I moved RuleGroupList to rulespb. I've rolled back the changes to errors.

Will merge once CI passes.

@pracucci pracucci merged commit ca89b03 into cortexproject:master Mar 12, 2021
@pracucci pracucci deleted the add-local-support-to-new-ruler-config branch March 12, 2021 08:53
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.

2 participants