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

Store Gateway: Add pre add block ownership check #6483

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Jan 6, 2025

The main issue this PR tries to solve can be found in thanos-io/thanos#8029

What this PR does:

Add block lifecycle pre add hook in Store Gateway to check block ownership again. If the block is not owned by the Store Gateway anymore due to ring change, don't try to sync the block.

I have deployed the change to one of our test environement and verified that the hook is able to not sync blocks that are not owned anymore.

caller=sharding_strategy.go:327 level=info org_id=xxx msg="block not owned from pre add check" block=01JC087F7K4AZFC53N0VHDYPA8

Which issue(s) this PR fixes:
Fixes #

Checklist

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

Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Copy link
Member

@alanprot alanprot left a comment

Choose a reason for hiding this comment

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

Very nice!

Thanks for doing this! LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 6, 2025
Signed-off-by: Ben Ye <[email protected]>
@yeya24 yeya24 merged commit 12412e6 into cortexproject:master Jan 6, 2025
17 checks passed
@yeya24 yeya24 deleted the block-rep-add-hook branch January 6, 2025 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/store-gateway lgtm This PR has been approved by a maintainer size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants