v12 chunk format handling in retention #5254
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also add some tests and safety mechanism to avoid dropping whole table when we cant find any chunks
owen-d
approved these changes
Jan 27, 2022
There was a problem hiding this comment.
Thanks for the quick PR. I'm worried about maintaining two distinct code paths for chunk key parsing in the repo because it makes it easy to forget one, diverge implementations, etc. Instead, I'd like to maintain one way to read chunks in pkg/storage/chunk which should help prevent these kind of issues in the future.
| if i == 0 { | ||
| return nil, nil, nil, false | ||
|
|
||
| // v12+ chunk id format `<user>/<fprint>/<start>:<end>:<checksum>` |
There was a problem hiding this comment.
I think we should standardize the internal functions used to parse chunks in pkg/storage/chunk so we don't have to maintain two versions, but that can be left for a later pr.
There was a problem hiding this comment.
Yes ! +1 Let's add an issue.
sandeepsukhani
added a commit
to sandeepsukhani/loki
that referenced
this pull request
Jan 27, 2022
Also add some tests and safety mechanism to avoid dropping whole table when we cant find any chunks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
While applying retention, compactor looks for chunk ids in specific formats. If it can't find any chunks it drops the whole table.
Since the recent
v12chunk id format is not recognized by the compactor, it drops the whole table while applying retention on table usingv12schema.This PR makes the retention code aware of
v12chunk id format and adds a safety mechanism to fail the retention operation when it can't find any chunk ids in the table.Checklist