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

index cleanup fixes while applying retention #4741

Merged

Conversation

sandeepsukhani
Copy link
Contributor

@sandeepsukhani sandeepsukhani commented Nov 12, 2021

What this PR does / why we need it:
This PR includes two fixes:

  1. Add a metric name if missing in labels used for calculating index entries for the series clean up since it is used while building the series IDs. The test did not catch this because the test data included the metric name label. Without the metric name, wrong series IDs were calculated, which caused labels entries not to get cleaned up.
  2. While calculating table interval from table name, we calculate the start time first and then add 24h to it. The problem with this is it would end up covering the start time of next days table. This is fixed by subtracting a millisecond from the end time of the interval after adding 24h to the start time. There was no harm caused by the overlap with the next day, I am just fixing it to avoid index entries being calculated unnecessarily for the next day as well.

Checklist

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

@sandeepsukhani sandeepsukhani requested a review from a team as a code owner November 12, 2021 09:31
@sandeepsukhani sandeepsukhani changed the title Retention index cleanup fixes index cleanup fixes while applying retention Nov 12, 2021
@sandeepsukhani sandeepsukhani force-pushed the retention-index-cleanup-fixes branch from a0197fa to d24cdf7 Compare November 12, 2021 09:42
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

Would be great to have regression tests.

@sandeepsukhani sandeepsukhani force-pushed the retention-index-cleanup-fixes branch from fc05e1b to 8436e81 Compare November 15, 2021 08:01
@sandeepsukhani sandeepsukhani merged commit a707ced into grafana:main Nov 15, 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