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

[S3 API] Support for S3 AbortMultipartUpload API #2940

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alyssaxu333
Copy link

@alyssaxu333 alyssaxu333 commented Nov 15, 2024

Summary

S3 Abort Multipart Upload API support
https://docs.aws.amazon.com/AmazonS3/latest/API/API_AbortMultipartUpload.html

Testing Done

Unit test, gradlew build

@@ -229,12 +231,13 @@ public void start() throws InstantiationException {
postAccountsHandler = new PostAccountsHandler(securityService, accountService, frontendConfig, frontendMetrics);
postDatasetsHandler = new PostDatasetsHandler(securityService, accountService, frontendConfig, frontendMetrics,
accountAndContainerInjector);
s3DeleteHandler = new S3DeleteHandler(deleteBlobHandler, frontendMetrics);
s3HeadHandler = new S3HeadHandler(headBlobHandler, securityService, frontendMetrics, accountService);
s3HeadHandler = new S3HeadHandler(headBlobHandler, securityService, frontendMetrics, accountService);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are there two s3headhandler?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't notice, just deleted.

s3HeadHandler = new S3HeadHandler(headBlobHandler, securityService, frontendMetrics, accountService);
s3HeadHandler = new S3HeadHandler(headBlobHandler, securityService, frontendMetrics, accountService);
s3MultipartUploadHandler =
new S3MultipartUploadHandler(securityService, frontendMetrics, accountAndContainerInjector, frontendConfig,
namedBlobDb, idConverter, router, quotaManager);
s3MultipartAbortHandler = new S3MultipartAbortUploadHandler(securityService, frontendMetrics, accountAndContainerInjector);
s3DeleteHandler = new S3DeleteHandler(deleteBlobHandler, s3MultipartAbortHandler, frontendMetrics);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to pass in s3MultipartUploadHandler to s3DeleteHandler, and branch the delete operation in s3MultipartUploadHandler.

Copy link
Author

@alyssaxu333 alyssaxu333 Nov 15, 2024

Choose a reason for hiding this comment

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

Yea I originally planned to do that. But s3MultipartUploadHandler doHandle() pass in a CallbackReadableStreamChannel , while s3DeleteHandler doHandle() passes in a CallbackVoid. If we wanna pass in s3MultipartUploadHandler, probably need to do more changes to s3DeleteHandler & deleteBlobHandler?

Comment on lines +65 to +67
if(S3MultipartUploadHandler.isMultipartAbortUploadRequest(restRequest)) {
multipartAbortHandler.handle(restRequest, restResponseChannel, callback);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we pass the s3MultipartUploadHandler here, then it would be multipartUploadHandler.handle(xxxx). Then inside of s3MultipartUploadHandler.doHandle method, we have if else statement to see if the request is an abort request.

@@ -57,6 +62,9 @@ public S3DeleteHandler(DeleteBlobHandler deleteBlobHandler, FrontendMetrics metr
*/
protected void doHandle(RestRequest restRequest, RestResponseChannel restResponseChannel, Callback<Void> callback)
throws RestServiceException {
if(S3MultipartUploadHandler.isMultipartAbortUploadRequest(restRequest)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it should be
if (xxxx) { // calling multipart handler } else { // calling objectHandler.handle }
otherwise, you would call objectHandler.handle even if it's multipart request.

@justinlin-linkedin
Copy link
Collaborator

Could you also change your title to be more descriptive?

@alyssaxu333 alyssaxu333 changed the title AbortMultipartUpload [S3 API] Support for S3 AbortMultipartUpload API Nov 15, 2024
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