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

allow applying retention at different interval than compaction with a config #4736

Conversation

sandeepsukhani
Copy link
Contributor

@sandeepsukhani sandeepsukhani commented Nov 11, 2021

What this PR does / why we need it:
When retention is enabled, we apply it with each compaction run. This causes a lot of resources to be wasted in a big Loki cluster which also causes compaction to not be able to keep up. Since applying retention is not as critical as compacting the data for better query performance, the apply retention interval can be set to a higher value than the compaction interval.

Special notes for your reviewer:
To keep it backwards compatible, I have allowed setting the apply retention interval to 0, which is the default. A zero value here means applying retention at the same time as compaction.
When a non-zero value is set, it should be a multiple of compaction interval because we would always apply retention when running compaction which is optimum and a pre-requisite for applying retention.

Checklist

  • Add an entry in the CHANGELOG.md about the changes.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

Not sure it needs to be a multiple, I guess this is a best effort.

@sandeepsukhani
Copy link
Contributor Author

Thanks for the review!

LGTM

Not sure it needs to be a multiple, I guess this is a best effort.

yeah it is a best-effort but requiring it to be multiple aligns the expectations of the user with the internal working of the compactor.

@sandeepsukhani sandeepsukhani merged commit 02d8846 into grafana:main Nov 12, 2021
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