tsdb/wlog: Remove any temproary checkpoints when creating a Checkpoint#16780
tsdb/wlog: Remove any temproary checkpoints when creating a Checkpoint#16780kgeckhart wants to merge 4 commits into
Conversation
bboreham
left a comment
bboreham
left a comment
There was a problem hiding this comment.
Thanks; this seems a valid idea though I am concerned that the logic seems incorrect.
2cdd761 to
c42d8ac
Compare
| if checkpoint.index >= maxIndex && !isTempCheckpoint(checkpoint) { | ||
| continue | ||
| } | ||
| // It's possible the latest checkpoint is a temporary checkpoint currently being created, so ignore the latest |
There was a problem hiding this comment.
Thanks for this.
To avoid adding this logic and have more/easier control on the tmps creation/deletion, don't you think it's more appropriate to add the cleaning here
prometheus/tsdb/wlog/checkpoint.go
Lines 130 to 132 in d902abc
Checkpoint() itself clean leftovers from its previous calls.
An extra iteration over the files shouldn't be that problematic. Also it'd be great if we could reuse removeBestEffortTmpDirs (we could move it under tsdbutil/ or sth)
There was a problem hiding this comment.
To avoid adding this logic and have more/easier control on the tmps creation/deletion, don't you think it's more appropriate to add the cleaning here
That makes sense, for some reason I thought DeleteCheckpoints was being called during initialization which could allow leftover checkpoints to be removed earlier but looks like it's only called after calling Checkpoint. It would be nice to try to delete them as early as possible since it's unlikely a call to Checkpoint happens on startup. LastCheckpoint is the closest to an "init" but it having a side effect of deleting files doesn't feel great. I could add another function to do it but that feels unnecessary as well.
Also it'd be great if we could reuse removeBestEffortTmpDirs (we could move it under tsdbutil/ or sth)
👍 I'll see what I can do here
There was a problem hiding this comment.
Having the logic in Checkpoint() where tmp things are already cleaned up + adding an exported method that only cleans tmp stuff for library users works for me as well.
c42d8ac to
d26c3bb
Compare
Signed-off-by: Kyle Eckhart <[email protected]>
Signed-off-by: Kyle Eckhart <[email protected]>
d26c3bb to
3975a38
Compare
Signed-off-by: Kyle Eckhart <[email protected]>
Signed-off-by: Kyle Eckhart <[email protected]>
kgeckhart
changed the title
kgeckhart
changed the title
|
I made a bad assumption on the refactor and need to do some more changes. I'll close this PR and re-open when I'm ready again. |
3 participants
This PR ensure
Checkpointwill remove any temporary checkpoint that might exist before creating a new one. This can help prevent accumulation of temporary checkpoints in the event a OOM/other shutdown case causes the process to shutdown while creating the checkpoint.Resolves: #16779
Note: Based on a suggestion I refactored the code used in
tsdb/db.goto cleanup temporary files in to a utility function shared between the two.