-
Notifications
You must be signed in to change notification settings - Fork 89
Add support for Android #460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will briefly create an empty file which could be read by a racing reader, violating atomicity
2d1fcea
to
6156595
Compare
I've addressed the comment by switching the implementation to use renameat2 with the I believe this addresses all the comments and gets this working on Android? |
Android doesn't support hard_link so provide an alternate path for performing the requested operation. This also fixes a bug where failed put might result in the staged file being left around.
6156595
to
13096ba
Compare
Is there some way we can test this? I presume we could add some build flag or something so we could test in CI on Linux without it becoming a public feature? I will also add that I am not sure what android versions will support this, it is on the newer end of Linux features. |
FWIW according to docs renameat2 is available from Linux 3.15 for ext4 so support for it is actually quite old I think (ext4 is the FS that Android tends to use). Can we merge this as is? I tested it by hand via lancedb and it works. I recognize there is some risk but I think that can be fixed by future commits & the risk is mitigated by this being configured out for non-Android platforms. |
I'm not comfortable merging code without some means of testing it, but it should be possible to construct something using a cfg flag or similar |
Maybe we could use an android emulator, similar to how we test using an s3 emulator |
It doesn't even need to be that complicated, the APIs exist on Linux, it just needs a config flag to switch to using them for the purposes of testing. |
Android doesn't support hard_link so provide an alternate path for performing the requested operation.
Which issue does this PR close?
Closes #459.
Rationale for this change
On Android hard links are not allowed. However, there are alternate ways to express atomicity of the same operations - we use OpenOptions::create_new instead to guarantee the target exists or not before clobbering it.
What changes are included in this PR?
LocalFilesystem put/copy/copy_if_not_exists alternatives for Android that don't use hard_link.
Are there any user-facing changes?
No.