Skip to content
This repository has been archived by the owner on Aug 13, 2019. It is now read-only.

Delete temp checkpoint folder on error. #415

Merged

Conversation

krasi-georgiev
Copy link
Contributor

No description provided.

@krasi-georgiev
Copy link
Contributor Author

cc @codesome

block.go Outdated
@@ -212,7 +213,7 @@ func writeMetaFile(dir string, meta *BlockMeta) error {

// Make any changes to the file appear atomic.
path := filepath.Join(dir, metaFilename)
tmp := path + ".tmp"
tmp := path + fileutil.TmpExt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of manually appending fileutil.TmpExt everywhere, would it be better to have a function call like fileutil.TmpDir which would take the dir and append fileutil.TmpExt to it? In case we want to change the logic of naming temp dir, this would come handy (example ioutil.TempDir).

package fileutil

func TmpDir(dir string) string {
    return dir + TmpExt
}
tmp := fileutil.TmpDir(path)

fileutil.TmpExt would still be present if it is needed somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I like the idea, but I prefer to be more explicit and tmp := fileutil.TmpDir(path) might be a bit strange. What does it do? create a tmp dir? copies to a tmp dir ?
refactoring is easy enough so lets keep as is and will change if needed.

checkpoint.go Outdated Show resolved Hide resolved
@krasi-georgiev
Copy link
Contributor Author

@gouthamve just when I was about to leave it to the windows users to fix 😜
this I realised it is an easy fix so now it passes for windows as well.

@krasi-georgiev krasi-georgiev force-pushed the chekcpont-delete-tmp branch 3 times, most recently from 1ae9c81 to ae479c8 Compare October 22, 2018 12:47
@krasi-georgiev
Copy link
Contributor Author

after having another look it is a terrible solution and very likely to brake but will keep it open as a reminder to think how ti fix properly for linux and windows.

Signed-off-by: Krasi Georgiev <[email protected]>
@krasi-georgiev
Copy link
Contributor Author

Refactored with a different approach. wal.Close() is now non blocking so it we just call it again and remove all tmp folder.

@krasi-georgiev
Copy link
Contributor Author

cc @codesome

@codesome
Copy link
Contributor

codesome commented Jan 5, 2019

LGTM

Copy link
Collaborator

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@krasi-georgiev krasi-georgiev merged commit 8d991bd into prometheus-junkyard:master Jan 7, 2019
@krasi-georgiev krasi-georgiev deleted the chekcpont-delete-tmp branch January 7, 2019 08:43
radeklesniewski pushed a commit to SabreOSS/tsdb that referenced this pull request Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants