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(services/s3): Add rename support for s3 service #4136

Closed
wants to merge 7 commits into from

Conversation

jihuayu
Copy link
Member

@jihuayu jihuayu commented Feb 4, 2024

I add rename support for the s3 service.
Because s3 doesn't have the native rename API, so we need to copy first and delete the old file.
I also added the rollback logic when we delete the old file failed.

@jihuayu jihuayu requested a review from Xuanwo as a code owner February 4, 2024 00:37
@github-actions github-actions bot requested review from xyjixyjixyji and oowl February 4, 2024 00:37
@jihuayu jihuayu marked this pull request as draft February 4, 2024 00:45
@jihuayu
Copy link
Member Author

jihuayu commented Feb 4, 2024

It seems like CI fail in main. I will re-open this PR after the CI passes.

@Xuanwo
Copy link
Member

Xuanwo commented Feb 4, 2024

Thanks for the PR. The capacity in backends is used to hint features that storage services support natively.

We don't need to implement rename in backend as we can do it at operator level.

For example: send rename if it's natively supported, otherwise fallback to copy and delete.

@jihuayu
Copy link
Member Author

jihuayu commented Feb 4, 2024

Thanks for the PR. The capacity in backends is used to hint features that storage services support natively.

We don't need to implement rename in backend as we can do it at operator level.

For example: send rename if it's natively supported, otherwise fallback to copy and delete.

OK, I will close this PR. Do we need to update the documentation to indicate that we do not support the rename operation for S3?

@Xuanwo
Copy link
Member

Xuanwo commented Feb 4, 2024

Do we need to update the documentation to indicate that we do not support the rename operation for S3?

I feel like it's clear enough the s3 doesn't support rename.

OK, I will close this PR

Would you like to implement the simulate logic for rename in CompleteLayer?

async fn rename(&self, from: &str, to: &str, args: OpRename) -> Result<RpRename> {
let capability = self.meta.full_capability();
if !capability.rename {
return Err(self.new_unsupported_error(Operation::Rename));
}
self.inner().rename(from, to, args).await
}

@jihuayu
Copy link
Member Author

jihuayu commented Feb 4, 2024

Would you like to implement the simulate logic for rename in CompleteLayer?

I will do it.
Do we treat it as the default behavior? Or do we need to add a flag to indicate the use of simulation?

I feel like it's clear enough the s3 doesn't support rename.

In our doc, there is no strikethrough in the s3 rename capability. But some others service has a strikethrough.
I think this could lead to misunderstandings that we simply do not currently support S3 rename operations temporarily.
So, I think we need to add a strikethrough to the s3 rename capability.
image
image

@Xuanwo
Copy link
Member

Xuanwo commented Feb 4, 2024

Do we treat it as the default behavior? Or do we need to add a flag to indicate the use of simulation?

We mark this different via full_capability and native_capability

@jihuayu jihuayu closed this Feb 4, 2024
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