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

refactor(services/aliyun-drive): directly implement oio::Write. #4821

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

yuchanns
Copy link
Member

@yuchanns yuchanns commented Jun 29, 2024

Related to #4780

Update

It's better to re-implement the oio::Write directly after some discussion with @Xuanwo:

Normally, we get the upload_id from initiate_part in the first call. When concurrency occurs, Aliyun Drive randomly fails on the method write_once which invokes initiate_part and has retries.

Oh, I see. The real issue here is that Aliyun Drive should not be treated as a valid multipart upload service.

A valid multipart upload service should be exactly the same with s3 that meets:

* An API such as `PutObject` can atomically create and upload a file.

* And a seperate group of APIs that can upload files in multiple parts: init, upload and complete.

Those two set of APIs should never be called in mixed. For example, wirte_once should never call init_multipart, it must be a single API call that create the file without extra states.

So back to our current issue. It's wrong to implement MultipartWrite::write_once by:

async fn write_once(&self, size: u64, body: crate::Buffer) -> Result<()> {
    let upload_id = self.initiate_part().await?;
    self.write_part(&upload_id, 0, size, body).await?;

    self.complete(&upload_id).await?;
    Ok(())
}

I suggest to implement oio::Write directly in such case.

Update End

After some investigation, I suspect the issue is about the retry behavior of the writer. So I made some changes to the writer.

@yuchanns yuchanns force-pushed the 4780 branch 2 times, most recently from 91606cf to 33d8344 Compare June 30, 2024 10:18
@yuchanns yuchanns changed the title fix(services/aliyun-drive): file exists caused by write retry refactor(services/aliyun-drive): directly implement oio::Write. Jun 30, 2024
@yuchanns yuchanns force-pushed the 4780 branch 2 times, most recently from 49db9bd to c29a6de Compare July 1, 2024 04:56
core/src/services/aliyun_drive/writer.rs Outdated Show resolved Hide resolved
core/src/services/aliyun_drive/writer.rs Outdated Show resolved Hide resolved
Signed-off-by: Hanchin Hsieh <me@yuchanns.xyz>
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Xuanwo Xuanwo merged commit 6634e5b into apache:main Jul 2, 2024
75 checks passed
@yuchanns yuchanns deleted the 4780 branch July 2, 2024 04:59
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.

2 participants