-
Notifications
You must be signed in to change notification settings - Fork 275
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
base: master
Are you sure you want to change the base?
Conversation
@@ -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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
if(S3MultipartUploadHandler.isMultipartAbortUploadRequest(restRequest)) { | ||
multipartAbortHandler.handle(restRequest, restResponseChannel, callback); | ||
} |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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.
Could you also change your title to be more descriptive? |
Summary
S3 Abort Multipart Upload API support
https://docs.aws.amazon.com/AmazonS3/latest/API/API_AbortMultipartUpload.html
Testing Done
Unit test, gradlew build