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: Add wasabi service implementation #2004

Merged
merged 6 commits into from
Apr 17, 2023
Merged

Conversation

darknight
Copy link
Contributor

This PR is for #1837

Changes include:

  • Added a new public API append in Operator struct. And accordingly, we added a stub in Accessor trait. This new interface caused cascading update for all LayeredAccessor, so changed all of them at one time.
    • Added OpAppend and RpAppend struct for request setting and response result, respectively.
    • Updated a few of enum definitions, add append related item.
  • Created wasabi module, and since wasabi is a S3 compatible service, so I borrowed most of logic directly from S3 implementation, with two more functionality added: rename & append, which means:
    • WasabiError resembles S3Error
    • WasabiPager resembles S3Pager
    • WasabiBuilder resembles S3Builder
    • WasabiBackend resembles S3Backend except that
      • It overrided Accessor::rename and Accessor::append interfaces
    • WasabiCore resembles S3Core except that
      • It added rename_object to integrate wasabi rename API (doc here). As discussed, we only consider file renaming for now.
      • It added append_object to integrate wasabi append API (doc here).
        • The official doc has no mention if it supports multipart append, so I emailed their support but no reply. But based on my trial, if we send POST to append API, just like what initiate_multipart_upload does, we've got error. So current append implementation is one single step operation.
    • WasabiWriter resembles S3Writer except that
      • It added OpAppend as struct field also
      • In write method, we differentiate append from multipart upload by OpAppend.enabled flag. Based on this flag, write will apply different methods from WasabiCore.

@Xuanwo
Copy link
Member

Xuanwo commented Apr 15, 2023

Hi, this PR seems include some not related changes which makes it hard to review. Can you rebase with main branch first?

@Xuanwo
Copy link
Member

Xuanwo commented Apr 15, 2023

Added a new public API append in Operator struct. 

append should not be added upon Operator. At least no in a PR which adds service support.

@Xuanwo Xuanwo marked this pull request as draft April 16, 2023 00:46
@darknight
Copy link
Contributor Author

Hi, this PR seems include some not related changes which makes it hard to review. Can you rebase with main branch first?

Done.

@darknight
Copy link
Contributor Author

Added a new public API append in Operator struct.

append should not be added upon Operator. At least no in a PR which adds service support.

Well, either I can help split append definition changes into another separate PR, or do you wish not have new API, instead resue existing interface?

@Xuanwo
Copy link
Member

Xuanwo commented Apr 16, 2023

do you wish not have new API, instead resue existing interface?

I prefer not to add new APIs.

@darknight
Copy link
Contributor Author

do you wish not have new API, instead resue existing interface?

I prefer not to add new APIs.

Ok, then perhaps I'll reuse write_with(path: &str, args: OpWrite, bs: impl Into<Bytes>) API in Operator instead of creating a new one. And in order to do that, I need to add one more flag field in OpWrite struct, to indicate whether current write is a append operation or normal write operation.
This way I suppose we can avoid to make change on Operator, Accessor and all related LayeredAccessors, sounds good?

@Xuanwo
Copy link
Member

Xuanwo commented Apr 16, 2023

And in order to do that, I need to add one more flag field in OpWrite struct, to indicate whether current write is a append operation or normal write operation.

Not needed, we already have append in OpWrite.

@darknight
Copy link
Contributor Author

hmm...the append field in OpWrite is for normal writing in multipart way (fetch upload_id if it's true), but the append we're talking about here is a separate operation, and I need another flag to differentiate append write from multipart write.

Let me update PR for better explanation.

@Xuanwo

This comment was marked as duplicate.

@Xuanwo

This comment was marked as duplicate.

@Xuanwo
Copy link
Member

Xuanwo commented Apr 16, 2023

hmm...the append field in OpWrite is for normal writing in multipart way (fetch upload_id if it's true), but the append we're talking about here is a separate operation,

This's true, service may have different ways to implement current Writer's API.

and I need another flag to differentiate append write from multipart write.

However, this way violates our VISION which makes users have to know whether a service supports multipart or append.


We just need to choose a method for implementing the append semantics provided by the Writer. In the future, we can offer different methods for users to select through the WasabiBuilder.

@darknight
Copy link
Contributor Author

darknight commented Apr 17, 2023

hmm...the append field in OpWrite is for normal writing in multipart way (fetch upload_id if it's true), but the append we're talking about here is a separate operation,

This's true, service may have different ways to implement current Writer's API.

and I need another flag to differentiate append write from multipart write.

However, this way violates our VISION which makes users have to know whether a service supports multipart or append.

We just need to choose a method for implementing the append semantics provided by the Writer. In the future, we can offer different methods for users to select through the WasabiBuilder.

Well, if we don't need support append write and multipart write at the same time, and leave out compatibility with S3 service, then I suppose the implementation is much easier and no new flag field is needed.

I pushed new changes, feel free to take a look, make sure I'm on the correct direction :P

@Xuanwo
Copy link
Member

Xuanwo commented Apr 17, 2023

I pushed new changes, feel free to take a look, make sure I'm on the correct direction :P

Thank you very much for your understanding and patience!

@Xuanwo Xuanwo changed the title Add wasabi service implementation feat: Add wasabi service implementation Apr 17, 2023
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! This PR LGTM now! The only thing left is to make our CI happy.

@Xuanwo
Copy link
Member

Xuanwo commented Apr 17, 2023

We can create a separate pull request to update our documentation.

Copy link
Member

@suyanhanx suyanhanx left a comment

Choose a reason for hiding this comment

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

Thanks for your great work!

@darknight darknight marked this pull request as ready for review April 17, 2023 04:34
@darknight darknight marked this pull request as draft April 17, 2023 04:36
@darknight darknight marked this pull request as ready for review April 17, 2023 04:54
@Xuanwo Xuanwo merged commit 7b0e3af into apache:main Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants