Move retention apply to the beginning of Compactor loop

bwplotka commented 4 years ago

We apply retention at the end, we probably should move it before any compaction. It was by design to make sure lower resolution can be downsampled, but TBH it does not really make much sense. If you configure the retention in a way that raw goes away too quickly this can hit you anyway. So I cannot see the blocker now (:

We can save a lot especially if retention is desired to be e.g 3 months but we left the data without compactor for 6 months (: We can avoid unnecessary compactions and downsampling.

AC:

Move retention applies to the beginning of Thanos compact loop.

GiedriusS commented 4 years ago

If you configure the retention in a way that raw goes away too quickly this can hit you anyway.

That's true but I would really love to make Thanos Compact scream loudly if this happens as it indicates some kind of error, no? Especially if downsampling is enabled but the retention policies prevent us from downsampling data.

bwplotka commented 4 years ago

I think we might need to make it a bit more complex.

Let's consider this case:

raw retention is 1month
downsampled retention is 2y
compactor is not running for 2 months.

Now what will happen with this change is that we will have a gap for downsampled data. between [2mo, 1mo] as we remove raw data that was dependent on.

I think we could improve the flow only for case where raw = res5m = res1h retention is equal. In this case, removal on the start might make sense.

Or in other words: We can only remove up to res1h retention at the start. Does it make sense? Other opinions are welcome (:

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

GiedriusS commented 4 years ago

Reopening since there is a new PR which tries to address this.

stale[bot] commented 4 years ago

This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions.

kakkoyun commented 4 years ago

Still ongoing. Let's keep it fresh for a little more.

stale[bot] commented 4 years ago

This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions.

stale[bot] commented 4 years ago

Closing for now as promised, let us know if you need this to be reopened! 🤗

GiedriusS commented 4 years ago

In progress.

stale[bot] commented 4 years ago

Hello 👋 Looks like there was no activity on this issue for last 30 days. Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗 If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

stale[bot] commented 4 years ago

Closing for now as promised, let us know if you need this to be reopened! 🤗

lzh-lab commented 3 years ago

Maybe this issue should be more attention.

stale[bot] commented 3 years ago

Hello 👋 Looks like there was no activity on this issue for the last two months. Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗 If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

stale[bot] commented 3 years ago

Hello 👋 Looks like there was no activity on this issue for the last two months. Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗 If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

heyitsmdr commented 3 years ago

Is this still on-going by any chance?