Uh oh!
There was an error while loading. Please reload this page.
HDDS-1213. Support plain text S3 MPU initialization request - #549
Conversation
hadoop-yetus
commented
Mar 4, 2019
💔 -1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
hadoop-yetus
commented
Mar 4, 2019
💔 -1 overall
This message was automatically generated. |
This comment has been minimized.
This comment has been minimized.
Over all patch LGTM. Few minor comments:
I see below error when running test manually with docker ozones3 cluster on my dev machine. I see when allocating a buffer, I am getting this error. I see by default we allocate 2 64mb buffer for all buffer lists. Any idea how to resolve this? |
| Run Keyword if '${system}' == 'Linux' Create Random file for linux | ||
| ${result} = Execute AWSS3APICli upload-part --bucket ${BUCKET} --key multipartKey --part-number 1 --body /tmp/part1 --upload-id ${nextUploadID} | ||
| Should contain ${result} ETag | ||
| ${system} = Evaluate platform.system() platform |
There was a problem hiding this comment.
We can remove line 55, as we have previously checked system type and created random file.
Now it is removed, we don't need it.
| ${result} = Execute AWSS3APICli upload-part --bucket ${BUCKET} --key multipartKey1 --part-number 1 --body /tmp/part1 --upload-id ${uploadID} | ||
| ${eTag1} = Execute and checkrc echo '${result}' | jq -r '.ETag' 0 | ||
| Should contain ${result} ETag | ||
| ${system} = Evaluate platform.system() platform |
There was a problem hiding this comment.
We can remove this line. Same comment as above
| Should contain ${result} ETag | ||
| ${result} = Execute AWSS3APICli upload-part --bucket ${BUCKET} --key multipartKey1 --part-number 2 --body /tmp/part2 --upload-id ${uploadID} | ||
| ${eTag2} = Execute and checkrc echo '${result}' | jq -r '.ETag' 0 | ||
| Should contain ${result} ETag |
There was a problem hiding this comment.
Are these unintended changes? As I see the change is indentation.
There was a problem hiding this comment.
Yetus asked me to not use tab. Without fixing the tabs the patch would cause a whitespace mismatch (my new lines with spaces and the old lines with tabs). So it's intentional, but I can move to a different
| Should contain ${result} ETag | ||
| ${result} = Execute AWSS3APICli upload-part --bucket ${BUCKET} --key multipartKey2 --part-number 2 --body /tmp/part2 --upload-id ${uploadID} | ||
| ${eTag2} = Execute and checkrc echo '${result}' | jq -r '.ETag' 0 | ||
| Should contain ${result} ETag |
There was a problem hiding this comment.
Can we remove these indentation changes?
| ${result} = Execute AWSS3APICli upload-part --bucket ${BUCKET} --key multipartKey5 --part-number 2 --body /tmp/part2 --upload-id ${uploadID} | ||
| ${eTag2} = Execute and checkrc echo '${result}' | jq -r '.ETag' 0 | ||
| Should contain ${result} ETag | ||
| ${system} = Evaluate platform.system() platform |
elek
commented
Mar 6, 2019
White space problems are reported by the yetus. I committed to a fix in a separated commit (a086953) to make it easier to review. I removed the platform lines (9f59f5d), but to be honest, they are also independent from the patch. So we should either accept both whitespace/platform fixes or create two new jiras for both. Just to be consistent ;-) (As they are both committed in two separated commits they can be reverted/accepted during the merge.) |
bharatviswa504
commented
Mar 6, 2019
Hi @elek
But platform change is related to this patch, as previosuly we used to see check platform type and run random create file. As now you have changed it, so we don't need the check. So, that is the reason for the comments. +1 LGTM. |
bharatviswa504
commented
Mar 6, 2019
@elek |
elek
commented
Mar 6, 2019
bq. But platform change is related to this patch, as previously we used to see check platform type and run random create file. Oh, I got it finally. You are right. I changed it to use raw bytes instead of M or m postfixes. |
This comment has been minimized.
This comment has been minimized.
vivekratnavel
commented
Mar 7, 2019
I am hitting the same error as @bharatviswa504 @elek Please find attached the log from docker-compose |
elek
commented
Mar 7, 2019
@vivekratnavel thanks to check it I tried to reproduce the problem I uploaded 100MB file from my host machine with/without the patch and it worked well. It's also working from acceptance tests both on my side and on the jenkins. Can you please share more details? How do you run the tests? What is the os? What is your awscli/docker version. Do you have any memory limitation on the virtual host which runs docker? Thanks |
elek
commented
Mar 7, 2019
Can you please try to increase your allocated memory for the containers? |
bharatviswa504
commented
Mar 8, 2019
Thank You @elek for the info. |
Author: Cameron Lee <calee@linkedin.com> Reviewers: Yi Pan <nickpan47@gmail.com> Closesapache#549 from cameronlee314/sync_li_trunk
S3 Multi-Part-Upload (MPU) is implemented recently in the Ozone s3 gateway. We have extensive testing with using 'aws s3api' application which is passed.
But it turned out that the more simple
aws s3 cpcommand fails with 405 Media type not supported error messageThe root cause of this issue is the JAXRS implementation of the multipart upload method:
{code}
@post
@produces(MediaType.APPLICATION_XML)
public Response multipartUpload(
@PathParam("bucket") String bucket,
@PathParam("path") String key,
@QueryParam("uploads") String uploads,
@QueryParam("uploadId") @DefaultValue("") String uploadID,
CompleteMultipartUploadRequest request) throws IOException, OS3Exception {
if (!uploadID.equals("")) {
//Complete Multipart upload request.
return completeMultipartUpload(bucket, key, uploadID, request);
} else {
// Initiate Multipart upload request.
return initiateMultipartUpload(bucket, key);
}
}
{code}
Here we have a CompleteMultipartUploadRequest parameter which is created by the JAXRS framework based on the media type and the request body. With Content-Type: application/xml it's easy: the JAXRS framework uses the built-in JAXB serialization. But with plain/text content-type it's not possible as there is no serialization support for CompleteMultipartUploadRequest from plain/text.
See: https://issues.apache.org/jira/browse/HDDS-1213