Fix filehandling for windows #392
Conversation
Signed-off-by: Goutham Veeramachaneni <[email protected]>
|
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 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() | |||
There was a problem hiding this comment.
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() | |||
Windows: Close files before deleting Checkpoints. Signed-off-by: Goutham Veeramachaneni <[email protected]> Windows: Close writers in case of errors so they can be deleted Signed-off-by: Goutham Veeramachaneni <[email protected]> Windows: Close block so that it can be deleted. Signed-off-by: Goutham Veeramachaneni <[email protected]> Windows: Close file to delete it Signed-off-by: Goutham Veeramachaneni <[email protected]> Windows: Close dir so that it can be deleted. Signed-off-by: Goutham Veeramachaneni <[email protected]> Windows: close files so that they can be deleted. Signed-off-by: Goutham Veeramachaneni <[email protected]>
|
@brian-brazil @fabxc @krasi-georgiev PTAL. These are non-trivial and might break things, I'm working on testing this now. |
gouthamve
force-pushed
the
fix-windows
branch
2 times, most recently
from
b38fdc9 to
d3649c9
Compare
|
WAL migration and compactions work on Windows now. Tested with |
|
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()) | |||
There was a problem hiding this comment.
Why are you calling hf.Close twice?
gouthamve
force-pushed
the
fix-windows
branch
from
d3649c9 to
e68f826
Compare
|
👍 |
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. |
There was a problem hiding this comment.
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
gouthamve
force-pushed
the
fix-windows
branch
from
e68f826 to
a5e8eaf
Compare
Signed-off-by: Goutham Veeramachaneni <[email protected]>
|
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! |
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