-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
HADOOP-18339. fix storage class option doesn't work with heap and byte buffer #4669
Conversation
🎊 +1 overall
This message was automatically generated. |
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.
+1, LGTM
@steveloughran @mukund-thakur would you be able to take a look? We also have fixing test pr #4489 related to storage class option and ready for review |
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.
LGTM. some comments.
was that setCannedAcl missing from dir markers?
PutObjectRequest putObjectRequest = new PutObjectRequest(getBucket(), key, | ||
inputStream, metadata); | ||
setOptionalPutRequestParameters(putObjectRequest); | ||
putObjectRequest.setCannedAcl(cannedACL); |
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.
were we missing this before?
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.
no, it was in newPutObjectRequest(..)
before, but I extracted it here because calling that function will also set storage class to directory marker
return Arrays.asList(new Object[][]{ | ||
{FAST_UPLOAD_BUFFER_DISK}, | ||
{FAST_UPLOAD_BUFFER_ARRAY}, | ||
{FAST_UPLOAD_BYTEBUFFER} |
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.
let's cut the bytebuffer one as we know the array option gives us the coverage we need, and it will save test time
thanks Steve, I have updated the pr. and no the setCannedAcl wasn't missing, I just extracted it from another function. |
🎊 +1 overall
This message was automatically generated. |
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.
+1
thanks for this; especially as I'm cherrypicking things internally this week.
while those parameterized tests are great for exploring the configuration space, they are a dangerously easy way to expand test time...it's good to be as efficient as we can while still getting that coverage up
…writes to disk. (#4669) Follow-up to HADOOP-12020 Support configuration of different S3 storage classes; S3 storage class is now set when buffering to heap/bytebuffers, and when creating directory markers Contributed by Monthon Klongklaew
…writes to disk. (apache#4669) Follow-up to HADOOP-12020 Support configuration of different S3 storage classes; S3 storage class is now set when buffering to heap/bytebuffers, and when creating directory markers Contributed by Monthon Klongklaew
Description of PR
HADOOP-18339. S3A storage class option only picked up when buffering writes to disk.
The problem is that there are two
createPutObjectRequest
classes, one for source file and another one for input stream. Previously, only the first one is picking up storage class option because I thought the second one is used only by creating directory marker which should not have any storage class.This is fixed it by making both of
createPutObjectRequest
classes pick up the storage class option and updatingnewDirectoryMarkerRequest
to create PUT request on its own then add parameterized test to verify it.How was this patch tested?
Tested with a bucket in
eu-west-1
withmvn -Dparallel-tests -DtestsThreadCount=16 clean verify
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?