Skip to content

Conversation

@reinersinger
Copy link
Contributor

@reinersinger reinersinger commented Jun 26, 2024

Introduce extended NFS File System interface.

The functions in the extended file system interface get additional context information for the call like uid/gid.
This makes sense, if the file storage is implemented in a different server component, that is able to execute
tasks like permission checking on its own.

The original NFS File System interface is not changed, but it is called from the default implementation of the extended interface. This ensures downward compatibility, so existing implementations do not break.

New implementations still could use the original interface,
which is easier to use, if the extended functionality is not required.

@ylow ylow requested a review from hoytak June 27, 2024 16:55
@schreter
Copy link

schreter commented Jul 3, 2024

I tried to comment on the change via Reviewable, but unfortunately, it is blocking me from doing so. Something in the project settings doesn't allow it, so I can't post the review. Not sure what to do.

It says: "Failed to publish: GitHub error 403 on POST https://api.github.com/repos/xetdata/nfsserve/pulls/29/reviews: Although you appear to have the correct authorization credentials, the xetdata organization has enabled OAuth App access restrictions, meaning that data access to third-parties is limited. For more information on these restrictions, including how to enable this app, visit https://docs.github.com/articles/restricting-access-to-your-organization-s-data/".

@ylow and @hoytak any idea? Thanks.

@reinersinger reinersinger marked this pull request as ready for review July 11, 2024 13:26
@reinersinger
Copy link
Contributor Author

@ylow and @hoytak, do you think, this could work?

Extending the original interface would also be possible, but the question then is how to deal with incompatibilities
for existing applications.

Details of this approach could certainly be discussed ...

@ylow
Copy link
Contributor

ylow commented Aug 9, 2024

Apologies as you might have heard we just got acquired by HuggingFace. Things have been hectic for a while, but now I have bandwidth to review this and get it in. Thanks for the patience!

@rrauch
Copy link
Contributor

rrauch commented Aug 21, 2024

If think this would be better as a breaking change instead of having two separate VFS traits. Additional context would be a genuinely useful addition. This would also be in line with other VFS interfaces (e.g. Fuse).

Reusing RPCContext might make sense here - minus the vfs & mount_signal fields (they could simply be made more private).

@reinersinger
Copy link
Contributor Author

reinersinger commented Nov 15, 2024

Adding an extended interface has also other advantages.
For server side implementations, the pre op and post op attributes could also be fetched internally within the same call.
This does not make such a big difference, if the NFSFilesystem trait is served within the same process. But if calls
are delegated there to a separate server component additional network roundtrips are involved.

But we could also start by adding additional parameters to the existing NFSFilesystem trait and try to optimize later on,
if performance is getting an issue. @ylow, what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants