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.

Sign up for GitHub

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

Jump to bottom

feat: add Writer::abort method #1937

Merged
merged 5 commits into from
Apr 14, 2023
Merged

feat: add Writer::abort method #1937

merged 5 commits into from
Apr 14, 2023

Conversation

v0y4g3r
Copy link
Contributor

@v0y4g3r v0y4g3r commented Apr 13, 2023 •
edited
Loading

This PR:

  • adds oio::Write::abort API to allow cancellation of an on-going append writer
  • for those writers do not support append, adds a Write::abort implementation that simply return Ok(())
  • adds delegated implementations of oio::Write::abort for all layers
  • adds oio::Write::abort implementation for S3Writer as an example

This PR does not add an equivalent API in oio::BlockingWrite, since we can always add a Drop implementation for blocking writers and delete partial objects in Drop::drop.

Related issue

#1607

@Xuanwo
Copy link
Member

Xuanwo commented Apr 14, 2023

Hi, @v0y4g3r, is this PR ready for review? Do you want to add abort API first and then add support for different services?

@v0y4g3r
Copy link
Contributor Author

v0y4g3r commented Apr 14, 2023

Hi, @v0y4g3r, is this PR ready for review? Do you want to add abort API first and then add support for different services?

Maybe I can add S3 implementation along with the Writer::abort definition in this PR so that any one who is interested in that can involve in.

@Xuanwo
Copy link
Member

Xuanwo commented Apr 14, 2023

Maybe I can add S3 implementation along with the Writer::abort definition in this PR so that any one who is interested in that can involve in.

LGTM!

@v0y4g3r v0y4g3r marked this pull request as ready for review April 14, 2023 07:13
Xuanwo
Xuanwo reviewed Apr 14, 2023
View reviewed changes
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.

Mostly LGTM!

core/src/services/oss/writer.rs Outdated Show resolved Hide resolved
core/src/services/hdfs/writer.rs Outdated Show resolved Hide resolved
core/src/services/fs/writer.rs Outdated Show resolved Hide resolved
core/src/raw/oio/write.rs Outdated Show resolved Hide resolved
core/src/raw/adapters/kv/backend.rs Outdated Show resolved Hide resolved
Xuanwo
Xuanwo approved these changes Apr 14, 2023
View reviewed changes
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!

@v0y4g3r
Copy link
Contributor Author

v0y4g3r commented Apr 14, 2023

I'm wondering if we can add a service test for Write::abort?

@Xuanwo
Copy link
Member

Xuanwo commented Apr 14, 2023

I'm wondering if we can add a service test for Write::abort?

Yes, we can add abort in behavior test (by ignoring not support errors)! This is PR good to get merged. How about adding a new PR for that?

@Xuanwo Xuanwo merged commit 1ec735e into apache:main Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xuanwo Xuanwo Xuanwo approved these changes

@PsiACE PsiACE Awaiting requested review from PsiACE

@ClSlaid ClSlaid Awaiting requested review from ClSlaid

@sundy-li sundy-li Awaiting requested review from sundy-li

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@v0y4g3r @Xuanwo