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

feat bucket replicate: added flag to ignore blocks marked for deletion #5117

Merged
merged 5 commits into from
Feb 7, 2022

Conversation

FUSAKLA
Copy link
Member

@FUSAKLA FUSAKLA commented Feb 3, 2022

Hi, we bumped into some race conditions with tools bucket replicate from bucket with a compactor running, such as replication of a block that just gets deleted by compactor.
Also, this can save some bandwidth, since it's mostly useless to sync blocks that will be deleted eventually.

Would you be ok accepting this? I'm open to any suggestions., thanks!

Changes

  • added a new flag --ignore-marked-for-deletion to the thanos bucket replicate command

Verification

  • added tests
  • tested manually

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Please rebase on top of newest main, those whitespace changes in docs/... are not needed.

pkg/replicate/scheme.go Show resolved Hide resolved
pkg/replicate/scheme_test.go Outdated Show resolved Hide resolved
cmd/thanos/tools_bucket.go Show resolved Hide resolved
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
@FUSAKLA
Copy link
Member Author

FUSAKLA commented Feb 3, 2022

Thanks, @GiedriusS for the swift review!

The white space changes were caused by the make docs, not sure why. Rolled it back.

@FUSAKLA
Copy link
Member Author

FUSAKLA commented Feb 3, 2022

Passing through the docs test is a task on its own I see, might consider making it the default, so I do not have to add any flags 😄

GiedriusS
GiedriusS previously approved these changes Feb 4, 2022
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

LGTM, let's see for a few days if anyone has any other suggestions

pkg/replicate/scheme.go Outdated Show resolved Hide resolved
@wiardvanrij
Copy link
Member

Only got one question, other than that, LGTM! :)

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
@FUSAKLA
Copy link
Member Author

FUSAKLA commented Feb 7, 2022

Not sure why but the PR diff broke GitHub somehow 😅
Getting 500 on the changes page..

Copy link
Member

@wiardvanrij wiardvanrij left a comment

Choose a reason for hiding this comment

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

LGTM

@GiedriusS GiedriusS merged commit de2bfd6 into thanos-io:main Feb 7, 2022
Nicholaswang pushed a commit to Nicholaswang/thanos that referenced this pull request Mar 6, 2022
thanos-io#5117)

* feat bucket replicate: added flag to ignore blocks marked for deletion

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>

* cr: fix docs and test

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>

* cr: add debug log on failed deletion mark read

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>

* cr: return error instead of logging issues with reading deletion marks

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Nicholaswang <wzhever@gmail.com>
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.

3 participants