Skip to content

Conversation

@ivandika3
Copy link
Contributor

@ivandika3 ivandika3 commented Apr 6, 2024

What changes were proposed in this pull request?

HDDS-6440 added support for custom metadata for normal object creation.

We should also support custom metadata creation for multipart upload key.

The metadata should be set during the multipart upload initiation request, which will be committed during the multipart upload complete request.

See: https://docs.aws.amazon.com/cli/latest/reference/s3api/create-multipart-upload.html

Should be tested using unit tests and acceptance tests.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10653

How was this patch tested?

Unit, integration, and acceptance tests.

Clean CI: https://github.com/ivandika3/ozone/actions/runs/8581916553

@ivandika3
Copy link
Contributor Author

ivandika3 commented Apr 6, 2024

@kerneltime @DaveTeng0 @SaketaChalamchala @vtutrinov Could you help take a look when you have time?

@adoroszlai adoroszlai added the s3 S3 Gateway label Apr 8, 2024
@kerneltime
Copy link
Contributor

cc @SaketaChalamchala @tanvipenumudy @vtutrinov can you'll please take a look?

@vtutrinov
Copy link
Contributor

@ivandika3 thanks for the changes. LGTM at first glance, going to continue the review a little bit later

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @ivandika3 for the patch.

Comment on lines 45 to 48
import static org.mockito.ArgumentMatchers.anyMap;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyBoolean;
import static org.mockito.Mockito.anyLong;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: import from Mockito.

Suggested change
import static org.mockito.ArgumentMatchers.anyMap;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyBoolean;
import static org.mockito.Mockito.anyLong;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyBoolean;
import static org.mockito.Mockito.anyLong;
import static org.mockito.Mockito.anyMap;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. Updated.

Comment on lines 3695 to 3698
for (Map.Entry<String, String> customEntry : customMetadata.entrySet()) {
assertTrue(keyMetadata.containsKey(customEntry.getKey()));
assertEquals(customEntry.getValue(), keyMetadata.get(customEntry.getKey()));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use assertThat instead of assertTrue for better message in case of failure. It also provides assertions for maps.

Suggested change
for (Map.Entry<String, String> customEntry : customMetadata.entrySet()) {
assertTrue(keyMetadata.containsKey(customEntry.getKey()));
assertEquals(customEntry.getValue(), keyMetadata.get(customEntry.getKey()));
}
assertThat(keyMetadata).containsAllEntriesOf(customMetadata);

Copy link
Contributor Author

@ivandika3 ivandika3 Apr 13, 2024

Choose a reason for hiding this comment

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

Thank you for the suggestion. Updated with assertThat (also updated for other similar tests).

I'll try to use more assertThat APIs in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some test failure due to the last commit, let me fix it first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@vtutrinov
Copy link
Contributor

@ivandika3 thanks for the patch, LGTM, +1

@ivandika3 ivandika3 self-assigned this Apr 16, 2024
@ivandika3 ivandika3 requested a review from adoroszlai April 18, 2024 01:47
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @ivandika3 for updating the patch.

Comment on lines 214 to 215
String multipartUploadID = keyToMultipartUpload.get(key).getUploadId();
if (multipartUploadID == null || !multipartUploadID.equals(uploadID)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't keyToMultipartUpload.get(key) == null indicate "no such upload? We'd hit NPE in that case. I guess MultipartInfoStub#uploadID would rarely be null.

(createMultipartKey seems to have the correct logic.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for catching it. I have updated it as the same logic as createMultipartKey.

@adoroszlai adoroszlai merged commit d10a822 into apache:master Apr 22, 2024
@adoroszlai
Copy link
Contributor

Thanks @ivandika3 for the patch, @vtutrinov for the review.

@ivandika3
Copy link
Contributor Author

Thank you @vtutrinov and @adoroszlai for the reviews.

ivandika3 added a commit to ivandika3/ozone that referenced this pull request May 6, 2024
ivandika3 added a commit to ivandika3/ozone that referenced this pull request May 6, 2024
ivandika3 added a commit to ivandika3/ozone that referenced this pull request May 7, 2024
ivandika3 added a commit to ivandika3/ozone that referenced this pull request May 7, 2024
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request May 29, 2024
ivandika3 added a commit to ivandika3/ozone that referenced this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s3 S3 Gateway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants