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

Per-tenant retention periods #3879

Merged
merged 9 commits into from
Mar 1, 2021

Conversation

stevesg
Copy link
Contributor

@stevesg stevesg commented Feb 25, 2021

What this PR does:
Implement per-tenant retention periods by performing block deletion in the Compactor.

Note that I've split the change into three commits to make reviewing it easier:

  1. Add the new configuration
  2. Add the new metrics
  3. The change + unit tests

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

Checklist

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

@stevesg stevesg changed the title Per user retention Per-tenant retention periods Feb 25, 2021
@stevesg stevesg marked this pull request as ready for review February 25, 2021 12:50
@pracucci pracucci requested review from pstibrany and pracucci and removed request for pstibrany February 25, 2021 15:09
Copy link
Contributor

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

Very good job 👏 👏 👏 The PR LGTM! I left few nits, but everything should be quick to address 🚀

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/util/validation/limits.go Outdated Show resolved Hide resolved
pkg/util/validation/limits.go Outdated Show resolved Hide resolved
pkg/compactor/compactor.go Outdated Show resolved Hide resolved
pkg/compactor/blocks_cleaner_test.go Outdated Show resolved Hide resolved
pkg/compactor/blocks_cleaner.go Outdated Show resolved Hide resolved
pkg/compactor/blocks_cleaner.go Outdated Show resolved Hide resolved
pkg/compactor/blocks_cleaner.go Outdated Show resolved Hide resolved
pkg/compactor/blocks_cleaner_test.go Outdated Show resolved Hide resolved
pkg/compactor/blocks_cleaner_test.go Outdated Show resolved Hide resolved
Signed-off-by: Steve Simpson <steve.simpson@grafana.com>
Signed-off-by: Steve Simpson <steve.simpson@grafana.com>
Signed-off-by: Steve Simpson <steve.simpson@grafana.com>
Signed-off-by: Steve Simpson <steve.simpson@grafana.com>
Signed-off-by: Steve Simpson <steve.simpson@grafana.com>
Signed-off-by: Steve Simpson <steve.simpson@grafana.com>
Signed-off-by: Steve Simpson <steve.simpson@grafana.com>
@stevesg
Copy link
Contributor Author

stevesg commented Feb 26, 2021

Rebase

Copy link
Contributor

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

Good job, LGTM! (modulo a couple of nits)

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/util/validation/limits.go Outdated Show resolved Hide resolved
pkg/util/validation/limits.go Outdated Show resolved Hide resolved
pkg/util/validation/limits.go Outdated Show resolved Hide resolved
pkg/compactor/blocks_cleaner.go Outdated Show resolved Hide resolved

// applyUserRetentionPeriod marks blocks for deletion which have aged past the retention period.
func (c *BlocksCleaner) applyUserRetentionPeriod(ctx context.Context, idx *bucketindex.Index, userID string, userBucket *bucket.UserBucketClient, userLogger log.Logger) error {
retention := c.cfgProvider.CompactorRetentionPeriod(userID)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, but the log below will be misleading ("applying retention"). I think we should also check if retention <= 0 to circuit break.

pkg/compactor/blocks_cleaner.go Outdated Show resolved Hide resolved
Signed-off-by: Steve Simpson <steve.simpson@grafana.com>
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, nice job. Thank you!

pkg/compactor/blocks_cleaner.go Outdated Show resolved Hide resolved
pkg/compactor/blocks_cleaner.go Outdated Show resolved Hide resolved
pkg/compactor/compactor.go Show resolved Hide resolved
Signed-off-by: Steve Simpson <steve.simpson@grafana.com>
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.

Thanks.

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.

Feature request: be able to specify the retention time for blocks
3 participants