cleanup tmp files

[zhulongcheng]

Closes #565

[krasi-georgiev]

Here is another related bug which we should fix here or in a separate PR
prometheus/prometheus#4058 (comment)

ping me when ready for a review.

[krasi-georgiev]

how is it going with this one?

I received you ping on IRC and replied, but maybe you were offline. I suggest using Riot to never miss IRC messaged.

[zhulongcheng]

I suggest using Riot to never miss IRC messaged.

Thanks Krasi

[krasi-georgiev]

did you test that

if err := os.RemoveAll(to); err != nil {
		return err
	}

is not needed?

[krasi-georgiev]

btw @zhulongcheng don't forget that tomorrow the 9th is the last day for the GSOC proposal submissions. Feel free to ping me on IRC with yours so I can give some feedback before submitting.

[krasi-georgiev]

a file conflict and did you test that

if err := os.RemoveAll(to); err != nil {
		return err
	}

is not needed?

[zhulongcheng]

Will rebase ASAP.

About test:
For replacing files, os.RemoveAll is not needed.
For replacing dirs, os.RemoveAll is required to ensure the dest dir not exists.

[krasi-georgiev]

so IIUC we do need it since fileutil.Rename should also handle dirs.

[zhulongcheng]

so IIUC we do need it since fileutil.Rename should also handle dirs.

How about fileutil.Replace?
https://github.com/prometheus/tsdb/blob/4b3a5ac5d36e5262d2656c8d149e137c2d1fab12/fileutil/fileutil.go#L128-L149

[krasi-georgiev]

do you mean to use Rename only for files and Replace for files or dirs?

is there no way to combine these into a single func?

[krasi-georgiev]

actually maybe best to keep separate, this way when Rename will fail if the destination exists and Replace will delete destination and rename source to destination.

[zhulongcheng]

Maybe we need to reconsider whether we still need the fileutil.Rename function?

If the destination file/dir exists, should use fileutil.Replace.
If the destination file/dir not exists, use fileutil.Replace, and os.RemoveAll (in fileutil.Replace func) just does nothing.

[krasi-georgiev]

I am concerned that this might lead to unexpected data loss. For example if the generated destination ULID of a block collapses with an existing block Replace will just delete the destination without us even knowing about this leading to data loss.

[zhulongcheng]

I want to simplify this pr.
just do two things:

1. replace renameFile with fileutil.Replace, and remove the renameFile func.
2. cleanup tmp files.

because the implementation of renameFile is identical to fileutil.Replace, the replace operation is safe.

this will make this pr clear and be reviewed friendly.

[zhulongcheng]

new a separate issue to discuss implementations and usages of Rename and Replace.

[krasi-georgiev]

ok

[krasi-georgiev]

if #583 turns out to be a good solution to inject FS errors can use it to write a test for this PR.

[krasi-georgiev]

LGTM
no need to wait for #583 as it would probably take a while and we can write that test later.

ping @gouthamve , @codesome , @bwplotka

[krasi-georgiev]

since it is a low risk PR , will merge and if anymore changes are needed will open a new one.

[bwplotka]

LGTM