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

compact: Added support for no-compact markers in planner. #3409

Merged
merged 2 commits into from
Nov 6, 2020

Conversation

bwplotka
Copy link
Member

@bwplotka bwplotka commented Nov 4, 2020

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 @SuperQ

Signed-off-by: Bartlomiej Plotka bwplotka@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 next iteration.

Fixes: #1424

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@bwplotka
Copy link
Member Author

bwplotka commented Nov 4, 2020

Example no-compact-mark.json: {"id":"00000000030000000000000000","version":1,"reason":"index-size-exceeding","details":"yolo"}

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.

Only several nits. Amazing work!

pkg/compact/compact.go Outdated Show resolved Hide resolved
pkg/compact/planner.go Outdated Show resolved Hide resolved
// 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.
Copy link
Contributor

@yeya24 yeya24 Nov 5, 2020

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?

Copy link
Member Author

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?

Copy link
Contributor

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

Copy link
Member Author

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 🤗

Copy link
Contributor

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 👍

pkg/compact/planner.go Show resolved Hide resolved
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@bwplotka
Copy link
Member Author

bwplotka commented Nov 6, 2020

Addressed comments @yeya24 PTAL (:

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.

LGTM!

@bwplotka bwplotka merged commit 8de5e29 into master Nov 6, 2020
@bwplotka bwplotka deleted the compmarker branch November 6, 2020 16:00
Oghenebrume50 pushed a commit to Oghenebrume50/thanos that referenced this pull request Dec 7, 2020
…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>
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.

compactor - "write postings: exceeding max size of 64GiB"
2 participants