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.

Sign up for GitHub

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

Jump to bottom

tools: Added remove flag on bucket mark command to remove deletion, no-downsample or no-compact markers on the block #5977

Merged
merged 1 commit into from
Dec 21, 2022
Merged

tools: Added remove flag on bucket mark command to remove deletion, no-downsample or no-compact markers on the block #5977

merged 1 commit into from
Dec 21, 2022

Conversation

maheshbaliga
Copy link
Contributor

@maheshbaliga maheshbaliga commented Dec 16, 2022 •
edited
Loading

Closes #5946

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Added remove flag on thanos tools bucket mark command to remove the deletion, no-downsample and no-compact markers that were added on the block.

Verification

Tested the changes by building the CLI and running the new command to check if the marker files were deleted. Also, added unit tests for the new functionality.

fpetkovski
fpetkovski previously approved these changes Dec 16, 2022
View reviewed changes
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Thanks, one small comment, but it looks good to me 👍

pkg/block/block.go Outdated
return errors.Wrapf(err, "check if %s file exists in bucket", markedFile)
}
if !markedFileExists {
level.Warn(logger).Log("msg", "requested to unmark, but file does not exist; this should not happen; investigate", "err", errors.Errorf("file %s does not exist in bucket", markedFile))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
level.Warn(logger).Log("msg", "requested to unmark, but file does not exist; this should not happen; investigate", "err", errors.Errorf("file %s does not exist in bucket", markedFile))
level.Warn(logger).Log("msg", "requested to unmark, but file does not exist", "err", errors.Errorf("file %s does not exist in bucket", markedFile))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@yeya24
Copy link
Contributor

yeya24 commented Dec 16, 2022

We just added the new No downsample marker so please also add that.

@maheshbaliga maheshbaliga force-pushed the add-bucket-unmark-command branch 2 times, most recently from 0675b47 to fcbd5ba Compare December 16, 2022 09:26
@maheshbaliga maheshbaliga changed the title tools: Added bucket unmark command to remove deletion or no-compact markers on the block tools: Added bucket unmark command to remove deletion, no-downsample or no-compact markers on the block Dec 16, 2022
@maheshbaliga
Copy link
Contributor Author

@yeya24. Included no-downsample marker as well

@maheshbaliga maheshbaliga force-pushed the add-bucket-unmark-command branch 3 times, most recently from 187290f to 4e0d60f Compare December 16, 2022 10:00
@yeya24
Copy link
Contributor

yeya24 commented Dec 17, 2022

Overall this pr looks good. But I am just kind of unsure if we want a new command unmark. What about just using the original mark command and add a flag to undo?

@maheshbaliga
Copy link
Contributor Author

@yeya, I thought having a --remove flag on the mark command would be confusing. Also, --details flag on mark is a required flag, which is not applicable for the unmark. Hence decided to add a new command. Let me know how to proceed.

@yeya24
Copy link
Contributor

yeya24 commented Dec 19, 2022 •
edited
Loading

Also, --details flag on mark is a required flag, which is not applicable for the unmark

We can just make it optional if it is unmark operation.

Let me know how to proceed.

Either option is ok to me but would love to hear more from the community and other maintainers.

yeya24
yeya24 previously approved these changes Dec 19, 2022
View reviewed changes
@maheshbaliga
Copy link
Contributor Author

Hi @fpetkovski, Any thoughts on whether this functionality should be a separate command or another flag on the existing command?

@fpetkovski
Copy link
Contributor

I think adding a --remove flag on the existing mark command would be great. It also reads nicely when you type thanos tools mark --remove

@maheshbaliga maheshbaliga changed the title tools: Added bucket unmark command to remove deletion, no-downsample or no-compact markers on the block tools: Added remove flag on bucket mark command to remove deletion, no-downsample or no-compact markers on the block Dec 21, 2022
@maheshbaliga maheshbaliga force-pushed the add-bucket-unmark-command branch 2 times, most recently from 536f067 to c2003e5 Compare December 21, 2022 04:54
@maheshbaliga
Tools: Added remove flag on bucket mark command to remove deletion, n…
112fc5f 
…o-downsample or no-compact markers on the blocks.

Signed-off-by: maheshbaliga <mahesh.baliga@infracloud.io>
@maheshbaliga maheshbaliga requested review from yeya24 and removed request for fpetkovski and yeya24 December 21, 2022 05:26
@maheshbaliga maheshbaliga requested review from fpetkovski and yeya24 and removed request for fpetkovski and yeya24 December 21, 2022 05:27
@maheshbaliga
Copy link
Contributor Author

Hi @yeya24, @fpetkovski I have made the required changes. Kindly re-review the same.

yeya24
yeya24 approved these changes Dec 21, 2022
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks!

fpetkovski
fpetkovski approved these changes Dec 21, 2022
View reviewed changes
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution 👍

@yeya24 yeya24 merged commit 5761afb into thanos-io:main Dec 21, 2022
ngraham20 pushed a commit to ngraham20/thanos that referenced this pull request May 18, 2023
@maheshbaliga
Tools: Added remove flag on bucket mark command to remove deletion, n…
e22ccf7 
…o-downsample or no-compact markers on the blocks. (thanos-io#5977)

Signed-off-by: maheshbaliga <mahesh.baliga@infracloud.io>

Signed-off-by: maheshbaliga <mahesh.baliga@infracloud.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpetkovski fpetkovski fpetkovski approved these changes

@yeya24 yeya24 yeya24 approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow removing markers from object storage
3 participants
@maheshbaliga @yeya24 @fpetkovski