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

Fix filehandling for windows #392

Merged
merged 3 commits into from
Sep 21, 2018
Merged

Fix filehandling for windows #392

merged 3 commits into from
Sep 21, 2018

Conversation

gouthamve
Copy link
Collaborator

Windows requires it's files to be closed before being removed. We've been bitten by this before.

Having tests doesn't help if we don't run them :/ See: prometheus/prometheus#4622

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Copy link
Collaborator Author

I created this PR on my Mac because of all git credentials and everything. But turns out the tests still fail on Windows and I think Windows was always broken, even in 2.3.x.

Will debug them early tomorrow. Any help is appreciated.

wal.go Outdated
@@ -1272,7 +1272,6 @@ func MigrateWAL(logger log.Logger, dir string) (err error) {
if err != nil {
return errors.Wrap(err, "open old WAL")
}
defer w.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's an error, below removing this could mean the wal is left open

wal/wal.go Outdated
@@ -311,16 +311,18 @@ func (w *WAL) Repair(origErr error) error {
if err != nil {
return errors.Wrap(err, "open segment")
}
defer f.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Windows: Close files before deleting Checkpoints.

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

Windows: Close writers in case of errors so they can be deleted

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

Windows: Close block so that it can be deleted.

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

Windows: Close file to delete it

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

Windows: Close dir so that it can be deleted.

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

Windows: close files so that they can be deleted.

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Copy link
Collaborator Author

@brian-brazil @fabxc @krasi-georgiev PTAL. These are non-trivial and might break things, I'm working on testing this now.

@gouthamve gouthamve force-pushed the fix-windows branch 2 times, most recently from b38fdc9 to d3649c9 Compare September 20, 2018 11:45
@gouthamve
Copy link
Collaborator Author

WAL migration and compactions work on Windows now. Tested with min-block-duration set to 2m.

@gouthamve
Copy link
Collaborator Author

And checkpointing works on Windows! I think this is good to go!

wal.go Outdated
@@ -735,10 +742,12 @@ func (w *SegmentWAL) Close() error {
// On opening, a WAL must be fully consumed once. Afterwards
// only the current segment will still be open.
if hf := w.head(); hf != nil {
return errors.Wrapf(hf.Close(), "closing WAL head %s", hf.Name())
if err := hf.Close(); err != nil {
return errors.Wrapf(hf.Close(), "closing WAL head %s", hf.Name())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you calling hf.Close twice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Typo, fixed.

@brian-brazil
Copy link
Contributor

👍

compact.go Outdated
// We are explicitly closing them here to check for error even
// though these are covered under defer. This is because in Windows,
// you cannot delete these unless there is they are closed and we want to close them
// in case of any errors above.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is because in Windows,
you cannot delete these unless there is they are closed and we want to close them
in case of any errors above.

I don't understand this comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Copy link
Collaborator Author

I've let this run overnight on Darwin, Linux and Windows. I've checked the migration again on windows and darwin. Everything runs pretty well, so this is good to go!

@gouthamve gouthamve merged commit 9c8ca47 into master Sep 21, 2018
@gouthamve gouthamve deleted the fix-windows branch September 21, 2018 05:31
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