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

Compact userTSDB's OOO head when compacting head #4180

Merged
merged 5 commits into from
Feb 9, 2023

Conversation

npazosmendez
Copy link
Contributor

@npazosmendez npazosmendez commented Feb 7, 2023

What this PR does

This adds a call to db.CompactOOOHead as part of userDb.compactHead, which means the OOO head is also compacted apart from the in-order head. As a consequence, compactions caused by idling DBs or forced head compactions will now also compact the OOO head, see code below:

mimir/pkg/ingester/ingester.go

Lines 2196 to 2203 in 6852e94

case force:
reason = "forced"
err = userDB.compactHead(i.cfg.BlocksStorageConfig.TSDB.BlockRanges[0].Milliseconds())
case i.compactionIdleTimeout > 0 && userDB.isIdle(time.Now(), i.compactionIdleTimeout):
reason = "idle"
level.Info(i.logger).Log("msg", "TSDB is idle, forcing compaction", "user", userID)
err = userDB.compactHead(i.cfg.BlocksStorageConfig.TSDB.BlockRanges[0].Milliseconds())

Which issue(s) this PR fixes or relates to

Fixes #4160

Checklist

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

This fixes a bug where the OOO head was not being
compacted for an idle TSDB or when the compaction
was forced.
@CLAassistant
Copy link

CLAassistant commented Feb 7, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

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

This looks fine to me. Let's fix the test and get this merged.

This PR also needs a CHANGELOG entry.

pkg/ingester/ingester_test.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_test.go Outdated Show resolved Hide resolved
pkg/ingester/ingester_test.go Outdated Show resolved Hide resolved
@npazosmendez
Copy link
Contributor Author

@pstibrany thanks for the feedback, test looks cleaner now. I've added a changelog entry, but you may be able to phrase it better since you have more context on the issue this was causing. Feel free to push any changes and merge if you think it's ready (I'll be on PTO for the rest of the week and I don't want to block this)

@npazosmendez npazosmendez marked this pull request as ready for review February 9, 2023 07:04
@npazosmendez npazosmendez requested a review from a team as a code owner February 9, 2023 07:04
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Copy link
Member

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

Thank you very much. I've rephrased changelog entry a bit, but we cannot merge until you sign the Contributor License Agreement: https://cla-assistant.io/grafana/mimir?pullRequest=4180.

I've enabled automerge, once you sign the CLA.

@pstibrany pstibrany enabled auto-merge (squash) February 9, 2023 10:51
@pstibrany pstibrany merged commit aee96e0 into main Feb 9, 2023
@pstibrany pstibrany deleted the fix-ooo-head-compaction branch February 9, 2023 13:29
@jesusvazquez
Copy link
Member

Nice work!! 💪

I believe this PR has also helped with #4160

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.

Ingester: idle or flush compaction do not compact OOO Head
4 participants