Fix ObjectStore.LocalFileSystem.put_opts for blobfuse #5094

RobinLin666 · 2023-11-18T07:07:17Z

Which issue does this PR close?

When we try to write to mounted path with blobfuse, we'll need to update object_store to explicitly close (drop) the file before calls to std::fs::rename, otherwise the metadata is not flushed in time for the rename.
Blobfuse may be for the sake of perf, so the upload file is only available when close file. Refer: https://github.com/Azure/azure-storage-fuse/blob/master/blobfuse/fileapis.cpp#L346

delta-io/delta-rs#1765

Rationale for this change

we'll need to update object_store to explicitly close (drop) the file before calls to std::fs::rename, otherwise the metadata is not flushed in time for the rename.

What changes are included in this PR?

sync the file before renaming.

Are there any user-facing changes?

no

tustvold · 2023-11-18T10:13:38Z

What is the motivation for using blobfuse over the first-class support Azure Storage, AFAICT fuse is just adding overhead (and race conditions)

tustvold · 2023-11-18T10:11:29Z

object_store/src/lib.rs

@@ -1064,6 +1064,8 @@ pub enum PutMode {
    /// Perform an atomic write operation if the current version of the object matches the
    /// provided [`UpdateVersion`], returning [`Error::Precondition`] otherwise
    Update(UpdateVersion),
+    /// Only for Mounted path, drop the file before renaming so that the file will upload.


This is a breaking API change is there some way we can avoid this?

maybe we can directly modify the option of Override?

You are adding a variant to public enumeration, this is a breaking change

RobinLin666 · 2023-11-18T13:26:09Z

What is the motivation for using blobfuse over the first-class support Azure Storage, AFAICT fuse is just adding overhead (and race conditions)

Mount is a very common usage scenario in Microsoft Fabric notebook. And it's use blobfuse to mount ADLS/OneLake storage. The benefits of using mount can be imagined. For example, users can use Azure Storage like local files. Uniform Access, Simplified Data Movement, Ease of Management, etc.

tustvold · 2023-11-18T13:33:58Z

The problem is blobfuse does not behave like a filesystem (or an object store). In particular, as was commented on the linked issue, hard links are not supported and rename is a non-atomic copy followed by a delete. LocalFilesystem relies on these primitives to provide object store behaviour using a filesystem. https://docs.rs/object_store/latest/object_store/#why-not-a-filesystem-interface might be relevant here.

I think you will need to use the object_store Azure abstraction, the performance will be orders of magnitude better and you won't run into these sorts of issues. Nothing prevents you from using blobfuse in addition within your notebooks, but workloads using object_store should be configured to talk to object storage directly

Edit: marking as draft for now

RobinLin666 · 2023-11-19T02:18:26Z

Hi @tustvold , thank you very much for your suggestion.
I think this line has no impact on the atomicity of the whole function, but it may affect the performance and error handling.

std::mem::drop(file);

This line releases the ownership of the file variable, but does not delete or modify the file itself. The file system is only affected when std::fs::rename or std::fs::hard_link are called. These operations are atomic, meaning they either succeed or fail without leaving any intermediate state. Therefore, the code is safe and does not cause file corruption or partial write.

However, this line may have some impact on the performance and error handling. If this line is not added, the file variable will be automatically dropped at the end of the function, which may delay some time. If this line is added, the file variable will be dropped immediately, which may improve some performance. But this also means that the file variable cannot be used for any further operations, such as checking for write errors, or closing the file.

BTW, the put method of object_store Azure abstraction also sends an http request, which is the same behavior as blobfuse, and blobfuse also sends some http requests. However, in this scenario, due to the implementation of local.rs, blobfuse will send some more requests, such as rename, etc. But this does not affect the atomicity of the whole put behavior. Yes, that’s right, in terms of performance, it may indeed be worse than using azure abstraction directly, but for some reason, users may indeed want to use mount to operate (some of the advantages of mount have been mentioned above) and do not care about this point of performance impact. I think we should support this user operation. What do you think?

If I say something wrong, please correct me, because I just started to learn arrow. Thanks!

tustvold · 2023-11-19T06:13:59Z

If there is a way to provide this without breaking existing workloads I suppose we could accomodate this. Although to be completely unambiguous I consider this an anti-pattern and would strongly discourage people from trying to pretend object stores are filesystems, it is a deeply problematic approach

tustvold · 2023-11-19T06:11:12Z

object_store/src/local.rs

@@ -368,7 +371,7 @@ impl ObjectStore for LocalFileSystem {
                return Err(err.into());
            }

-            let metadata = file.metadata().map_err(|e| Error::Metadata {
+            let metadata = metadata(&path).map_err(|e| Error::Metadata {


This is now racy in the event of a concurrent put

Sure, fixed.

tustvold · 2023-11-19T06:15:03Z

object_store/src/local.rs

+                            Ok(_) => None,
+                            Err(source) => Some(Error::UnableToRenameFile { source }),
+                        }
+                    }
                    PutMode::Create => match std::fs::hard_link(&staging_path, &path) {


Does this behave correctly on blobfuse, I'm not sure it does

Yes, blobfuse,s3fs and goofys all don't support hard link. But they can choose override mode.

RobinLin666 · 2023-11-19T10:29:58Z

If there is a way to provide this without breaking existing workloads I suppose we could accomodate this. Although to be completely unambiguous I consider this an anti-pattern and would strongly discourage people from trying to pretend object stores are filesystems, it is a deeply problematic approach

Thanks, I actually agree with what you said, putting a Filesystem on Top of an Object Store is a Bad Idea.
But sometimes users may not have high requirements for performance, or they may only be able to use the traditional POSIX protocol. This is also one of the meanings of mount. So I think we should not block this usage. Thanks

tustvold · 2023-11-19T18:16:56Z

object_store/src/local.rs

+                    e_tag = Some(get_etag(&metadata));
+                    match opts.mode {
+                        PutMode::Overwrite => {
+                            std::mem::drop(file);


Could we get a comment explaining the purpose of this to future readers, otherwise it is likely to be accidentally removed

Sure, thanks

tustvold · 2023-11-19T18:18:19Z

object_store/src/local.rs

-                            let _ = std::fs::remove_file(&staging_path); // Attempt to cleanup
-                            None
+                Ok(_) => {
+                    let metadata = file.metadata().map_err(|e| Error::Metadata {


Does this work with blobfuse, the original description suggested the file had to be closed before the metadata could be retrieved?

Yes, it works with blobfuse. Because when it sends stat system call, blobfuse will look for local file cache firstly and it will success.

Can you confirm if this works

Yes, I have tested it in local and it worked.

Hi @tustvold, you can see https://github.com/Azure/azure-storage-fuse/blob/master/blobfuse/utilities.cpp#L136

For metadate request, blobfuse will check local cache firstly and skip remote request. It's different with rename requests.

tustvold · 2023-11-26T20:28:12Z

Marking as draft as waiting on feedback, please feel free to mark ready for review when you would like me to take another look

RobinLin666 · 2023-11-27T01:22:29Z

Marking as draft as waiting on feedback, please feel free to mark ready for review when you would like me to take another look

Hi @tustvold , any question needs I answer/feedback? Please take a look again, thanks!

tustvold · 2023-11-27T06:38:36Z

#5094 (comment)

RobinLin666 · 2023-11-27T07:54:59Z

#5094 (comment)

Thanks, I have answered. Please check.

tustvold · 2023-11-27T10:08:25Z

I am not seeing anything 😅

tustvold · 2023-11-27T11:05:10Z

Review comments, listing as "pending", aren't submitted until you complete your review

Fix ObjectStore.LocalFileSystem.put_opts for blobfuse

5326dfb

github-actions bot added the object-store Object Store Interface label Nov 18, 2023

Fix ObjectStore.LocalFileSystem.put_opts for blobfuse

f107dc0

RobinLin666 marked this pull request as ready for review November 18, 2023 08:14

tustvold reviewed Nov 18, 2023

View reviewed changes

fix comment

2fffc37

tustvold added the api-change Changes to the arrow API label Nov 18, 2023

tustvold marked this pull request as draft November 18, 2023 21:16

tustvold removed the api-change Changes to the arrow API label Nov 18, 2023

tustvold reviewed Nov 19, 2023

View reviewed changes

fix race condition

f25ace3

tustvold reviewed Nov 19, 2023

View reviewed changes

add comment

f5677bf

RobinLin666 marked this pull request as ready for review November 20, 2023 02:09

tustvold marked this pull request as draft November 26, 2023 20:27

tustvold approved these changes Nov 27, 2023

View reviewed changes

tustvold marked this pull request as ready for review November 27, 2023 11:07

tustvold merged commit d5a6cf4 into apache:master Nov 27, 2023
13 checks passed

RobinLin666 mentioned this pull request Nov 27, 2023

When will upgrade object_store to 0.8? apache/datafusion#8330

Closed

mrchypark mentioned this pull request Sep 2, 2024

Add no_hardlinks option to LocalConfig and fix error handling splitgraph/seafowl#642

Closed

tustvold mentioned this pull request Sep 19, 2024

[ObjectStore] MultipartUpload::complete fails on WSL when writing to /mnt/ #6423

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ObjectStore.LocalFileSystem.put_opts for blobfuse #5094

Fix ObjectStore.LocalFileSystem.put_opts for blobfuse #5094

RobinLin666 commented Nov 18, 2023 •

edited

Loading

tustvold commented Nov 18, 2023

tustvold Nov 18, 2023

RobinLin666 Nov 18, 2023 •

edited

Loading

tustvold Nov 18, 2023

RobinLin666 commented Nov 18, 2023

tustvold commented Nov 18, 2023 •

edited

Loading

RobinLin666 commented Nov 19, 2023

tustvold commented Nov 19, 2023

tustvold Nov 19, 2023

RobinLin666 Nov 19, 2023

tustvold Nov 19, 2023

RobinLin666 Nov 19, 2023

RobinLin666 commented Nov 19, 2023

tustvold Nov 19, 2023

RobinLin666 Nov 20, 2023

tustvold Nov 19, 2023

RobinLin666 Nov 20, 2023

tustvold Nov 21, 2023

RobinLin666 Nov 22, 2023

RobinLin666 Nov 27, 2023

tustvold commented Nov 26, 2023

RobinLin666 commented Nov 27, 2023

tustvold commented Nov 27, 2023

RobinLin666 commented Nov 27, 2023

tustvold commented Nov 27, 2023

tustvold commented Nov 27, 2023

Fix ObjectStore.LocalFileSystem.put_opts for blobfuse #5094

Fix ObjectStore.LocalFileSystem.put_opts for blobfuse #5094

Conversation

RobinLin666 commented Nov 18, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold commented Nov 18, 2023

Choose a reason for hiding this comment

RobinLin666 Nov 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobinLin666 commented Nov 18, 2023

tustvold commented Nov 18, 2023 • edited Loading

RobinLin666 commented Nov 19, 2023

tustvold commented Nov 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobinLin666 commented Nov 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Nov 26, 2023

RobinLin666 commented Nov 27, 2023

tustvold commented Nov 27, 2023

RobinLin666 commented Nov 27, 2023

tustvold commented Nov 27, 2023

tustvold commented Nov 27, 2023

RobinLin666 commented Nov 18, 2023 •

edited

Loading

RobinLin666 Nov 18, 2023 •

edited

Loading

tustvold commented Nov 18, 2023 •

edited

Loading