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

Move retention apply to the beginning of Compactor loop #1708

Open
bwplotka opened this issue Nov 1, 2019 · 22 comments
Open

Move retention apply to the beginning of Compactor loop #1708

bwplotka opened this issue Nov 1, 2019 · 22 comments
Labels
component: compact dont-go-stale Label for important issues which tells the stalebot not to close them help wanted

Comments

@bwplotka
Copy link
Member

bwplotka commented Nov 1, 2019

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
Copy link
Member

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
Copy link
Member Author

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
Copy link

stale bot commented Jan 11, 2020

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
Copy link
Member

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

@GiedriusS GiedriusS reopened this Jan 29, 2020
@stale stale bot closed this as completed Feb 5, 2020
@GiedriusS GiedriusS reopened this Feb 5, 2020
@stale stale bot removed the stale label Feb 5, 2020
@stale
Copy link

stale bot commented Mar 6, 2020

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 stale bot added the stale label Mar 6, 2020
@stale stale bot closed this as completed Mar 13, 2020
@kakkoyun
Copy link
Member

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

@GiedriusS GiedriusS reopened this Mar 23, 2020
@stale stale bot removed the stale label Mar 23, 2020
@stale
Copy link

stale bot commented Apr 22, 2020

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 stale bot added the stale label Apr 22, 2020
@stale
Copy link

stale bot commented Apr 29, 2020

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

@stale stale bot closed this as completed Apr 29, 2020
@GiedriusS GiedriusS reopened this Apr 29, 2020
@stale stale bot removed the stale label Apr 29, 2020
@GiedriusS
Copy link
Member

In progress.

@stale
Copy link

stale bot commented May 29, 2020

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 stale bot added the stale label May 29, 2020
@stale
Copy link

stale bot commented Jun 5, 2020

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

@stale stale bot closed this as completed Jun 5, 2020
@lzh-lab
Copy link
Contributor

lzh-lab commented Dec 8, 2020

Maybe this issue should be more attention.

@stale
Copy link

stale bot commented Feb 6, 2021

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 stale bot added the stale label Feb 6, 2021
@kakkoyun kakkoyun added help wanted and removed stale labels Feb 9, 2021
@stale
Copy link

stale bot commented Jun 3, 2021

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 stale bot added the stale label Jun 3, 2021
@kakkoyun kakkoyun removed the stale label Jun 11, 2021
@heyitsmdr
Copy link

Is this still on-going by any chance?

@yeya24
Copy link
Contributor

yeya24 commented Jul 11, 2021

This can also address the problem mentioned in #4406

@stale
Copy link

stale bot commented Sep 11, 2021

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 stale bot added the stale label Sep 11, 2021
@stale
Copy link

stale bot commented Oct 12, 2021

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

@stale stale bot closed this as completed Oct 12, 2021
@yeya24 yeya24 reopened this Oct 13, 2021
@stale
Copy link

stale bot commented Oct 30, 2021

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

@stale stale bot closed this as completed Oct 30, 2021
@GiedriusS GiedriusS reopened this Oct 30, 2021
@stale stale bot removed the stale label Oct 30, 2021
@stale
Copy link

stale bot commented Jan 9, 2022

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 stale bot added the stale label Jan 9, 2022
@stale
Copy link

stale bot commented Mar 2, 2022

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

@stale stale bot closed this as completed Mar 2, 2022
@GiedriusS GiedriusS reopened this Mar 3, 2022
@stale stale bot removed the stale label Mar 3, 2022
@stale
Copy link

stale bot commented May 2, 2022

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 stale bot added the stale label May 2, 2022
@GiedriusS GiedriusS added dont-go-stale Label for important issues which tells the stalebot not to close them and removed stale labels May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: compact dont-go-stale Label for important issues which tells the stalebot not to close them help wanted
Projects
None yet
Development

No branches or pull requests

6 participants