Skip to content
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

feat: impl put_multipart in object_store #4793

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Rachelint
Copy link

@Rachelint Rachelint commented Jun 23, 2024

close: #4748

@Rachelint
Copy link
Author

@Xuanwo hi, have a moment to review?

}
}

impl fmt::Debug for OpendalUpload {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug has been included, so we can remove the usage of fmt. Or we can remove all Debug / Formatter instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -412,6 +417,74 @@ impl ObjectStore for OpendalStore {
}
}

/// `MultipartUpload`'s impl based on `Writer` in opendal
struct OpendalUpload {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about following the same naming pattern like object_store: OpendalMultiPartUpload?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// Refer to the fs impl in `object_store`:
// https://github.com/apache/arrow-rs/blob/a35214f92ad7c3bce19875bb091cb776447aa49e/object_store/src/local.rs#L715
// IO exists during locking, so use tokio::sync::Mutex here
writer: Arc<Mutex<Writer>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not take deep research on this API first.

Writer handles the write internally, all data must be written in order. Using a lock here is incorrect. Writer needs to make some changes to make it's possible to implement MultipartUpload.

Let me take a look first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @Rachelint. I'm currently working on a fix for this; let's mark this PR as a draft for now. Thanks for your patience.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @Rachelint. I'm currently working on a fix for this; let's mark this PR as a draft for now. Thanks for your patience.

Got it. Somethings I can help to fix? I am willing to do more and understand the project better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Somethings I can help to fix? I am willing to do more and understand the project better.

Hi, I'm still working on how to address this. Would you like to look at other issues first? Thanks for your understanding!

@Rachelint Rachelint force-pushed the impl-put-multipart branch 2 times, most recently from 819fce4 to d4391e3 Compare June 24, 2024 16:51
@Xuanwo Xuanwo marked this pull request as draft June 26, 2024 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

object_store: Implement put_multipart via Writer
2 participants