Fix WindowsFS to support Windows #14627
Conversation
…efor WindowsFS cannot be used on Windows. Due to Windows 11 no longer preventing depleting of open files, we need to be able to use WindowsFS on Windows (irony), fix this to fallback to realpath as key. It should be good enough like an inode. Renaming/moving files is already handles by WindowsFS when it detects a change in key after rename/move. Closes apache#11920
github-project-automation
bot
moved this to Open
in OpenSearch Lucene & Core Performance Tracking
|
Looks like it also passes with Windows 10 / Windows server 2022 (as used by Github). |
rmuir
left a comment
rmuir
left a comment
There was a problem hiding this comment.
Seems fine: thanks for fixing this.
|
@rmuir : It also works with the path (without realpath). Maybe we can just fallback to the path instead of realpath. It should be good enough, too and reduces overhead. What do you think? |
| } | ||
| // the key may be null, e.g. on real Windows! | ||
| // in that case we fallback to the real path | ||
| return existing.toRealPath(); |
There was a problem hiding this comment.
it also works with plain path. as we do not use symlinks on windows the extra toRealPath() seems to be useless overhead.
There was a problem hiding this comment.
@rmuir : You added thumbs up. Should I remove the realPath or leave it in? I feel better with realPath, but it may slowdown even more.
There was a problem hiding this comment.
We don't need to test for theoretical situations such as symlinks when we dont use them in lucene? So I would go with the fastest/simplest route, that solves our needs.
Its not a generic windows filesystem simulator, just good enough for our needs, to prevent deleting open files and test the logic surrounding that.
There was a problem hiding this comment.
and maybe the fileKey should have just never been used all along. There is a hardlinks functionality on linux, which was maybe why I tried to use inode...
There was a problem hiding this comment.
OK. I will change to plain path and only use filekey if it is available.
There was a problem hiding this comment.
and maybe the fileKey should have just never been used all along. There is a hardlinks functionality on linux, which was maybe why I tried to use inode...
Yes for hardlinks (we use them for addIndexes) it might be useful. So I made it to just fallback to the path if no fileKey is available. So we have best on both worlds. On Windows, here there are no hardlinks (or links at all without being root) the fallback to path solves all issues.
To me it was important: If WindowsFS is used on Posix, we won't cause unwanted failures.
|
I was able to remove the assume on more tests. Basically we should possibly also automatically enable WindowsFS randomly for Windows, too. This would allow to test legacy Windows behaviour also on Windows 11. Will add a followup commit once Github runner is happy. |
… also on Windows 11 / Windows Server 2024
|
Hi, For now I left the |
|
It also works on my local Windows 11 machine. |
|
I'll will merge this after the tests pass here. Let's see how it behaves long term. |
|
Theres one small problem that makes one test fail. ACtually the backslash is a reserved character (if used in Unix), but on Windows, it is actually the file separator. I will add a workaround, so on windows it allows backslash. |
github-project-automation
bot
moved this from Open
to Merged
in OpenSearch Lucene & Core Performance Tracking
|
I am out for lunch and only have my smartphone so can't cherry 🍒 pick to stable branch. Will do this when back at home, unless somebody wants to step in to silence Jenkins. |
Windows does not support file keys (the attribute returns null). Therefor WindowsFS cannot be used on Windows. Due to Windows 11 no longer preventing depleting of open files, we need to be able to use WindowsFS on Windows (irony), fix this to fallback to path as key. It should be good enough like an inode. Renaming/moving files is already handles by WindowsFS when it detects a change in key after rename/move. Closes #11920
I cherry-picked 67fdb91 @uschindler! Enjoy your lunch! |
Windows does not support file keys (the attribute returns null). Therefor WindowsFS cannot be used on Windows. Due to Windows 11 no longer preventing depleting of open files, we need to be able to use WindowsFS on Windows (irony), fix this to fallback to path as key. It should be good enough like an inode. Renaming/moving files is already handles by WindowsFS when it detects a change in key after rename/move. Closes #11920
5 participants
Windows does not support file keys (the attribute returns
nullwhich is documented by OpenJDK that it is optional). Therefore WindowsFS cannot be used on Windows.Due to Windows 11 no longer preventing deletion of open files, we need to be able to use
WindowsFSon Windows (irony), fix this to fallback to realpath as key. It should be good enough like an inode. Renaming/moving files is already handled by WindowsFS when it detects a change in key after rename/move.Closes #11920