-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
compact: Added support for no-compact markers in planner. #3409
Conversation
The planner algo was adapted to avoid unnecessary changes to blocks caused by excluded blocks, so we can quickly switch to different planning logic in next iteration. Fixes: #1424 Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only several nits. Amazing work!
pkg/compact/planner.go
Outdated
// We do not include a recently created block with max(minTime), so the block which was just created from WAL. | ||
// This gives users a window of a full block size to piece-wise backup new data without having to care about data overlap. | ||
|
||
// We do not include a recently created block with max(minTime), so the block which was just uploaded to bucket. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a block with the max minTime
is just uploaded? It is possible that it was uploaded several days ago, but has the max minTime
. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly what this comment says right? That it has max of minTimes of all blocks, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, if we stop adding new blocks to a bucket, and the latest block in the bucket was uploaded 3 days ago. So we also want to exclude the latest block in this case?
The comment mentions recently created block
so seems to check the block creation time makes more sense. Maybe I miss something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We mean the freshest TBH
also the whole check is really not super needed for us. It's relevant to Prometheus, but let's keep it 🤗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. If it is relevant to Prometheus then it makes sense 👍
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Addressed comments @yeya24 PTAL (: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…3409) * compact: Added support for no-compact markers in planner. The planner algo was adapted to avoid unnecessary changes to blocks caused by excluded blocks, so we can quickly switch to different planning logic in next iteration. Fixes: thanos-io#1424 Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Addressed comments. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Oghenebrume50 <raphlbrume@gmail.com>
The planner algo was adapted to avoid unnecessary changes to
blocks caused by excluded blocks, so we can quickly switch to
different planning logic in the next iteration.
Fixes: #1424
The idea is to have
no-compact-mark.json
in each block that has to be excluded from compaction. cc @SuperQSigned-off-by: Bartlomiej Plotka bwplotka@gmail.com