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

Global markers bucket now deletes both markers, and only reports "Exists" if both markers exists. #1015

Merged
merged 5 commits into from
Feb 4, 2022

Conversation

pstibrany
Copy link
Member

@pstibrany pstibrany commented Feb 3, 2022

What this PR does: This PR modifies globalMarkersBucket in two ways:

  • When calling Delete on block-local marker file, global marker file is always deleted too, even if block-local marker file doesn't exist.
  • When calling Exists on block-local marker file, it returns true only if global marker file exists as well. This helps MarkForDeletion and MarkForNoCompact functions from `github.com/thanos-io/thanos/pkg/block' pacakge to correctly re-upload marker files, if block-local marker already exists, but global marker file doesn't exist.

This fixes problem that appears in the log files as:

  • requested to mark for deletion, but file already exists; this should not happen; investigate
  • requested to mark for no compaction, but file already exists; this should not happen; investigate

Due to above logic, compactor was unable to clean up some blocks properly, which could cause that these blocks remain in the bucket as partial blocks.

Checklist

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

…s" if both files exist.

Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

I don't think I know enough about to Thanos to judge whether this change in behavior makes sense, otherwise the code looks good to me.

Signed-off-by: Peter Štibraný <[email protected]>
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Good job! I left a nit.

pkg/storage/tsdb/bucketindex/markers_bucket_client.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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