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

Update ignoreDeletionMarksDelay based on deleteDelay #61

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

clyang82
Copy link
Contributor

Set configure --ignore-deletion-marks-delay to (delete-delay)/2
Here is description from document:

I can configure --ignore-deletion-marks-delay in store (Duration after which the blocks marked for deletion will be filtered out while fetching blocks. The idea of ignore-deletion-marks-delay is to ignore blocks that are marked for deletion with some delay. This ensures store can still serve blocks that are meant to be deleted but do not have a replacement yet. If delete-delay duration is provided to compactor or bucket verify component, it will upload deletion-mark.json file to mark after what duration the block should be deleted rather than deleting the block straight away. If delete-delay is non-zero for compactor or bucket verify component, ignore-deletion-marks-delay should be set to (delete-delay)/2 so that blocks marked for deletion are filtered out while fetching blocks before being deleted from bucket. Default is 24h, half of the default value for --delete-delay on compactor)

Signed-off-by: clyang82 chuyang@redhat.com

securityContext: if std.objectHas(cr.spec, 'securityContext') then cr.spec.securityContext else obs.thanos.stores.config.securityContext,
ignoreDeletionMarksDelay: std.parseInt(std.substr(deleteDelay, 0, std.length(deleteDelay)-1))/2 + std.substr(deleteDelay, std.length(deleteDelay)-1, std.length(deleteDelay)),
Copy link
Member

Choose a reason for hiding this comment

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

this has some potential weird edge cases. If someone typed 1d for the deletion delay, then we would try to parse the int 1/2, which would result in 0d. This seems slightly dangerous to do in the operator code since the code runs automatically unlike when using plain jsonnet without an operator where users have the chance to review the generated manifests before applying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so do you mean that we should expose ignoreDeletionMarksDelay for customer to set?

Copy link
Member

Choose a reason for hiding this comment

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

yes, I think this would be a good way to start working around this for now. Otherwise we need to move parsing somewhere like golang

@clyang82 clyang82 requested a review from squat December 24, 2021 14:06
@clyang82 clyang82 force-pushed the ignoreDeletionMarksDelay branch 2 times, most recently from 9df4456 to f0fd4da Compare December 31, 2021 02:06
@clyang82
Copy link
Contributor Author

@squat PTAL. Thanks.

Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

Let's make sure these connects always end with a period for consistency

api/v1alpha1/observatorium_types.go Outdated Show resolved Hide resolved
api/v1alpha1/observatorium_types.go Outdated Show resolved Hide resolved
@clyang82 clyang82 requested a review from squat January 1, 2022 12:27
clyang82 and others added 5 commits January 4, 2022 09:30
Signed-off-by: clyang82 <chuyang@redhat.com>
Signed-off-by: clyang82 <chuyang@redhat.com>
Co-authored-by: Lucas Servén Marín <lserven@gmail.com>
Co-authored-by: Lucas Servén Marín <lserven@gmail.com>
Signed-off-by: clyang82 <chuyang@redhat.com>
@clyang82
Copy link
Contributor Author

clyang82 commented Jan 6, 2022

@squat PTAL

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.

2 participants