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 #116

Merged
merged 2 commits into from
Dec 17, 2024
Merged

Conversation

jnyi
Copy link
Collaborator

@jnyi jnyi commented Dec 17, 2024

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

}

// ApplyRetentionPolicyByTenant removes blocks depending on the specified retentionByTenant based on blocks MaxTime.
func ApplyRetentionPolicyByTenant(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

plan to add unit test for this as well

@jnyi jnyi requested review from hczhu-db and yuchen-db December 17, 2024 18:12
@@ -436,6 +436,12 @@ func runCompact(
level.Info(logger).Log("msg", "retention policy of 1 hour aggregated samples is enabled", "duration", retentionByResolution[compact.ResolutionLevel1h])
}

retentionByTenant, err := compact.ParesRetentionPolicyByTenant(logger, *conf.retentionTenants)
if err != nil && len(retentionByTenant) != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is len(retentionByTenant) != 0 needed here? If it's an empty config, err should already be nil.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch

"github.com/thanos-io/objstore"

"github.com/thanos-io/thanos/pkg/block"
"github.com/thanos-io/thanos/pkg/block/metadata"
)

const (
tenantRetentionRegex = `^([\w-]+):((\d{4}-\d{2}-\d{2})|(\d+d))$`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Give some examples in a piece of code comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

examples in unit tests already

Comment on lines 85 to 87
} else if matches[3] != "" {
policy.CutoffDate = cutoffDate
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: don't need else if{} because the function returns in if{}.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

still need this, matches[3] can be empty, we allow optional cutoffDate or a retention duration

Comment on lines 90 to 92
} else if matches[4] != "" {
policy.RetentionDuration = time.Duration(duration)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@jnyi jnyi force-pushed the pantheon-compactor-backlog branch from c106c13 to d2bd5b4 Compare December 17, 2024 20:57
Signed-off-by: Yi Jin <[email protected]>
@jnyi jnyi force-pushed the pantheon-compactor-backlog branch from d2bd5b4 to c3c1e59 Compare December 17, 2024 21:07
Copy link

@yuchen-db yuchen-db left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Yi Jin <[email protected]>
@jnyi jnyi force-pushed the pantheon-compactor-backlog branch from 56f4716 to 2cc4540 Compare December 17, 2024 23:07
@jnyi jnyi merged commit 95cb360 into databricks:db_main Dec 17, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants