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

Relax WriteMultipart API to support aborting after completion #5977

Closed
fsdvh opened this issue Jun 28, 2024 · 3 comments
Closed

Relax WriteMultipart API to support aborting after completion #5977

fsdvh opened this issue Jun 28, 2024 · 3 comments
Labels
enhancement Any new improvement worthy of a entry in the changelog object-store Object Store Interface

Comments

@fsdvh
Copy link
Contributor

fsdvh commented Jun 28, 2024

In the current implementation finish will consume our writer and in case of an error during the finish call we wound be able to abort our write.

I suggest changing the following:

pub async fn finish(mut self) -> Result<PutResult>

to something like this:

pub async fn finish(&mut self) -> Result<PutResult>

To be able to call the abort method after that

We potentially can safeguard ourselves from multiple finish calls by adding an is_finished flag to the writer

@fsdvh fsdvh added the enhancement Any new improvement worthy of a entry in the changelog label Jun 28, 2024
@tustvold
Copy link
Contributor

tustvold commented Jun 28, 2024

I think it is generally assumed that if finish errors the upload will be aborted and eventually cleaned up - https://docs.rs/object_store/latest/object_store/aws/index.html#multipart-uploads.

I think we could probably document more clearly that abort is an optimisation. We could also make WriteMultipart abort on failure

Edit: I'm pushing back on this largely out of a desire to avoid breaking API changes, which are problematic

@fsdvh
Copy link
Contributor Author

fsdvh commented Jul 2, 2024

So I made a change to make a cleanup automatic process during complete, wdyt @tustvold?

pub async fn finish(mut self) -> Result<PutResult> {
        if !self.buffer.is_empty() {
            let part = std::mem::take(&mut self.buffer);
            self.put_part(part.into())
        }

        self.wait_for_capacity(0).await?;

        match self.upload.complete().await {
            Err(e) => {
                self.tasks.shutdown().await;
                self.upload.abort().await?;
                Err(e)
            }
            Ok(result) => Ok(result),
        }
    }

@tustvold tustvold closed this as completed Jul 2, 2024
@alamb alamb added the object-store Object Store Interface label Jul 2, 2024
@alamb
Copy link
Contributor

alamb commented Jul 2, 2024

label_issue.py automatically added labels {'object-store'} from #5974

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any new improvement worthy of a entry in the changelog object-store Object Store Interface
Projects
None yet
Development

No branches or pull requests

3 participants