Skip to content

Refactor rulerstore packages #3887

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

Merged

Conversation

pracucci
Copy link
Contributor

What this PR does:
Similarly to the package refactoring done in #3881, in this PR I'm proposing the same refactoring (same structure) for the rulestore. This should help to better reorganize packages.

The diff is huge and I'm sorry for that, but most changes have been done with the Goland IDE refactoring tool and splitted in different commits:

  • Moved bucketclient implementation to pkg/ruler/rulestore/bucketclient
  • Moved local and objectclient rule store under pkg/ruler/rulestore/...
  • Renamed NewRuleStorage() to NewLegacyRuleStore()
  • Renamed pkg/ruler/rules to pkg/ruler/rulespb

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 February 26, 2021 14:25
@@ -31,28 +26,3 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
cfg.ConfigDB.RegisterFlagsWithPrefix(prefix, f)
cfg.RegisterFlagsWithPrefix(prefix, f)
}

// NewRuleStore creates a new bucket client based on the configured backend
func NewRuleStore(ctx context.Context, cfg Config, cfgProvider bucket.TenantConfigProvider, logger log.Logger, reg prometheus.Registerer) (rules.RuleStore, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to pkg/ruler/storage.go to avoid circular dependencies.

Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

YES! (Assuming test and lint pass)

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.

Awesome! LGTM besides an optional suggestion

Comment on lines 31 to 32
"github.com/cortexproject/cortex/pkg/ruler/rulespb"
store "github.com/cortexproject/cortex/pkg/ruler/rulespb"
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: we may be able to trim this down

Suggested change
"github.com/cortexproject/cortex/pkg/ruler/rulespb"
store "github.com/cortexproject/cortex/pkg/ruler/rulespb"
"github.com/cortexproject/cortex/pkg/ruler/rulespb"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely! I didn't notice it, thanks! (and done)

Copy link
Contributor

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

pracucci added 8 commits March 1, 2021 11:58
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci force-pushed the refactor-rulerstore-packages branch from d941008 to 44a3700 Compare March 1, 2021 10:59
@pracucci pracucci merged commit 282a092 into cortexproject:master Mar 1, 2021
@pracucci pracucci deleted the refactor-rulerstore-packages branch March 1, 2021 13:27
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