HDDS-9680. Use md5 hash of multipart object part's content as ETag #5668

vtutrinov · 2023-11-24T08:01:50Z

What changes were proposed in this pull request?

Replace the uploaded part's path with its content md5 hash for the part's ETag response field

In the scope of the HDDS-9115 HDDS-9114 jira tickets the feature to store key's ETag (content md5 hash) was implemented. But the ETag field in the response of the part loading request wasn't changed and showed the part's path as it takes part in completeMultipartUpload request (OM). The PR contains the next changes:

replace path-representation of the part's ETag by its content md5 hash
update org.apache.hadoop.ozone.om.request.s3.multipart.S3MultipartUploadCommitPartRequest in the scope of parts validating - now we validate the part's ETag instead of its partName
add eTag field to OmClientProtocol.proto/MultipartCommitUploadPartResponse
add eTag field to OmClientProtocol.proto/Part (to make completeMultipartUpload request)
add eTag field to OmClientProtocol.proto/PartInfo (listParts)

What is the link to the Apache JIRA

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

How was this patch tested?

Existing unit, integration, and smoke/acceptance/robot tests. The robot test smoketest/s3/MutipartUpload.robot/"Test Multipart Upload Complete" was patched to compare the part's ETag with md5 hash of their content

kerneltime · 2023-11-28T17:12:07Z

Can you address the merge conflicts?

…part upload)

…n tests

vtutrinov · 2023-11-29T07:01:39Z

Can you address the merge conflicts?

Done

kerneltime · 2023-11-29T23:32:32Z

cc @tanvipenumudy @SaketaChalamchala

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto

Make an 'eTag' field optional for Part and PartInfo messages

kerneltime · 2023-12-04T17:30:41Z

@SaketaChalamchala can you please take a look?

adoroszlai · 2023-12-15T11:51:09Z

@SaketaChalamchala @tanvipenumudy can you please review?

myskov · 2023-12-19T09:17:19Z

@SaketaChalamchala @tanvipenumudy please review

…-uploaded-part-eTag-improvement

...n/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java

kerneltime · 2024-01-04T20:52:31Z

The changes overall look good, we need to figure out how to handle older S3G connecting to OM and if there are any on going multipart uploads during upgrade. Open to discuss what is the right solution. This is a very important PR and we should try to get in asap.

SaketaChalamchala · 2024-01-05T01:39:02Z

The change LGTM

…-uploaded-part-eTag-improvement

…rovement

adoroszlai · 2024-01-25T09:41:59Z

...once my question about the proto.lock will be answered

Sorry, I didn't see your question, which question are you referring to?

@ivandika3 the question was from @adoroszlai - https://github.com/apache/ozone/pull/5668/files/48a1af2d2a82ab6c4780b2fb88502530ddf33362#r1464366194

I didn't see any question either.

vtutrinov · 2024-01-25T10:05:57Z

...once my question about the proto.lock will be answered

Sorry, I didn't see your question, which question are you referring to?

@ivandika3 the question was from @adoroszlai - https://github.com/apache/ozone/pull/5668/files/48a1af2d2a82ab6c4780b2fb88502530ddf33362#r1464366194

I didn't see any question either.

The notice from @adoroszlai was:

proto.lock should not be updated as part of the PR. The reason for having this file is to ensure backwards compatibility of any changes to .proto definitions. Lock files are used as the reference against which proto definitions are validated.

My reply to the notice above was:

How should we resolve the compatibility check failure in a maven build if the proto.lock won't be updated?

adoroszlai · 2024-01-25T15:12:57Z

The notice from @adoroszlai was:

proto.lock should not be updated as part of the PR. The reason for having this file is to ensure backwards compatibility of any changes to .proto definitions. Lock files are used as the reference against which proto definitions are validated.

My reply to the notice above was:

How should we resolve the compatibility check failure in a maven build if the proto.lock won't be updated?

@vtutrinov Thanks for re-posting, I don't see the same question anywhere else in PR.

Regarding the question: proto definitions need to be changed in a way to be backwards compatible.

For this PR, reverting the change of proto.lock, backwards compatibility check is still passing for me. Are you encountering some failure?

kerneltime

We should not update the proto.lock. The OmClientProtocol.proto file has new fields as optional and it should not need a change to the proto.lock.

…uests

…ences with the ref to it * replace "Md5" occurrences with the ref to OzoneConst.MD5_HASH const

adoroszlai · 2024-01-28T22:24:01Z

@vtutrinov please check test failures:

org.apache.hadoop.ozone.om.request.s3.multipart.TestS3MultipartUploadCompleteRequest
org.apache.hadoop.ozone.om.request.s3.multipart.TestS3MultipartUploadCompleteRequestWithFSO
org.apache.hadoop.ozone.om.response.s3.multipart.TestS3MultipartUploadCommitPartResponseWithFSO
org.apache.hadoop.ozone.om.response.s3.multipart.TestS3MultipartUploadCompleteResponseWithFSO

https://github.com/vtutrinov/ozone/actions/runs/7687119786/job/20946884097#step:5:2502

vtutrinov · 2024-01-29T06:29:17Z

@vtutrinov please check test failures:

org.apache.hadoop.ozone.om.request.s3.multipart.TestS3MultipartUploadCompleteRequest
org.apache.hadoop.ozone.om.request.s3.multipart.TestS3MultipartUploadCompleteRequestWithFSO
org.apache.hadoop.ozone.om.response.s3.multipart.TestS3MultipartUploadCommitPartResponseWithFSO
org.apache.hadoop.ozone.om.response.s3.multipart.TestS3MultipartUploadCompleteResponseWithFSO

https://github.com/vtutrinov/ozone/actions/runs/7687119786/job/20946884097#step:5:2502

Fixed

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto

…nal field (revert change)

lockfile removed

adoroszlai · 2024-01-29T07:44:37Z

Thanks a lot @vtutrinov for updating the patch.

ivandika3

Thank you @vtutrinov for addressing the reviews.

I also saw that you addressed some concerns posed by @kerneltime regarding the incomplete multipart upload backward compatibilities. Thanks for that.

I have some comments, but mostly LGTM. Thanks again for your hard work @vtutrinov.

ivandika3 · 2024-01-29T08:16:06Z

...n/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java

+        String dbPartETag = null;
+        String dbPartName = null;
+        if (partKeyInfo != null) {
+          dbPartETag = partKeyInfo.getPartKeyInfo().getMetadata(0).getValue();


Regarding this, I might miss something, but this assumes that the first element in the metadata is the eTag? Maybe we can explicitly get the metadata which key is equal to OzoneConsts.ETAG?

Currently it seems that only ETAG is the only metadata stored on the multipart upload part so it should be fine for now.

Thank you for the update.

ivandika3 · 2024-01-29T09:14:52Z

...n/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java

@@ -642,10 +661,36 @@ private String multipartUploadedKeyHash(
    StringBuffer keysConcatenated = new StringBuffer();
    for (PartKeyInfo partKeyInfo: partsList) {
      keysConcatenated.append(KeyValueUtil.getFromProtobuf(partKeyInfo
-          .getPartKeyInfo().getMetadataList()).get("ETag"));
+          .getPartKeyInfo().getMetadataList()).get(OzoneConsts.ETAG));


Regarding the incomplete multipart uploads compatibility, the parts that do not have "eTag" yet will return null. In StringBuffer, it will append four characters "null". However, I think there is little we can do here, so I think it should be fine to handle the incompatibility.

Thank you for the update. Instead of null, it will use partName if the md5 hash does not exist yet.

ivandika3 · 2024-01-29T09:17:01Z

...n/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java

+    boolean eTagBasedValidationAvailable = partsList.stream().allMatch(OzoneManagerProtocolProtos.Part::hasETag);
    // Now do actual logic, and check for any Invalid part during this.
    for (OzoneManagerProtocolProtos.Part part : partsList) {
      currentPartCount++;
      int partNumber = part.getPartNumber();
-      String partName = part.getPartName();
-
      PartKeyInfo partKeyInfo = partKeyInfoMap.get(partNumber);
-
-      String dbPartName = null;
-      if (partKeyInfo != null) {
-        dbPartName = partKeyInfo.getPartName();
-      }
-      if (!StringUtils.equals(partName, dbPartName)) {
-        String omPartName = partKeyInfo == null ? null : dbPartName;
+      MultipartCommitRequestPart requestPart = eTagBasedValidationAvailable ?
+          eTagBasedValidator.apply(part, partKeyInfo) : partNameBasedValidator.apply(part, partKeyInfo);
+      if (!requestPart.isValid()) {
        throw new OMException(
            failureMessage(requestedVolume, requestedBucket, keyName) +
-            ". Provided Part info is { " + partName + ", " + partNumber +
-            "}, whereas OM has partName " + omPartName,
+            ". Provided Part info is { " + requestPart.getRequestPartId() + ", " + partNumber +
+            "}, whereas OM has eTag " + requestPart.getOmPartId(),
            OMException.ResultCodes.INVALID_PART);


For my understanding: the idea seems to pass validation even if some of the completed parts do not have eTag field, in that case, we will only validate on the partName (since it always exist in both old and new versions), if all completed parts have eTag, the MPU complete will pass validation if the eTag metadata is equal to either the persisted partName and the eTag field (both partName and eTag should have the same value).

…x MPU commit request computing of eTag hash for old clients

ivandika3 · 2024-01-30T01:25:11Z

@vtutrinov thanks for updating the patch. LGTM +1.

adoroszlai · 2024-01-30T16:45:48Z

@kerneltime can you please take another look?

ivandika3 · 2024-02-06T01:42:29Z

@vtutrinov @adoroszlai While @kerneltime is reviewing this, could you help resolve the merge conflicts?

…-uploaded-part-eTag-improvement

adoroszlai · 2024-02-13T09:23:56Z

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

…pache#5668) (cherry picked from commit 7370676)

vtutrinov force-pushed the HDDS-9680-multipart-uploaded-part-eTag-improvement branch 2 times, most recently from 42cfbb9 to 9bcf0a1 Compare November 27, 2023 13:53

aswinshakil requested a review from kerneltime November 27, 2023 17:07

vtutrinov force-pushed the HDDS-9680-multipart-uploaded-part-eTag-improvement branch 2 times, most recently from d3e9f8f to 2ac2e76 Compare November 28, 2023 05:48

vtutrinov marked this pull request as ready for review November 28, 2023 15:35

kerneltime requested a review from duongkame November 28, 2023 17:10

Slava Tutrinov added 2 commits November 29, 2023 09:37

HDDS-9680. provide a loaded key part's ETag as an md5 hash (S3G multi…

66cee2b

…part upload)

HDDS-9680. tiny refactoring - move eTag computer to static variable i…

fd3d762

…n tests

vtutrinov force-pushed the HDDS-9680-multipart-uploaded-part-eTag-improvement branch from 2ac2e76 to fd3d762 Compare November 29, 2023 06:45

kerneltime reviewed Nov 29, 2023

View reviewed changes

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto Outdated Show resolved Hide resolved

kerneltime reviewed Nov 29, 2023

View reviewed changes

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto Outdated Show resolved Hide resolved

Update OmClientProtocol.proto

7e0ef01

Make an 'eTag' field optional for Part and PartInfo messages

adoroszlai requested a review from kerneltime November 30, 2023 17:17

adoroszlai changed the title ~~HDDS-9680. Provide a loaded key part's ETag as an md5 hash (S3G multipart upload)~~ HDDS-9680. Use md5 hash of multipart object part's content as ETag Nov 30, 2023

adoroszlai added the s3 S3 Gateway label Dec 1, 2023

adoroszlai added the needs review label Dec 15, 2023

Merge remote-tracking branch 'origin/master' into HDDS-9680-multipart…

d20291d

…-uploaded-part-eTag-improvement

kerneltime reviewed Jan 4, 2024

View reviewed changes

...n/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java Outdated Show resolved Hide resolved

adoroszlai and others added 2 commits January 5, 2024 21:46

Merge remote-tracking branch 'origin/master' into HDDS-9680-multipart…

0a17eb3

…-uploaded-part-eTag-improvement

Merge branch 'master' into HDDS-9680-multipart-uploaded-part-eTag-imp…

eb731bd

…rovement

adoroszlai removed the needs review label Jan 11, 2024

kerneltime requested changes Jan 26, 2024

View reviewed changes

Slava Tutrinov added 4 commits January 28, 2024 14:16

HDDS-9680. support ETag as an md5-hash for multipart upload parts req…

5eea2be

…uests

HDDS-9680. provide OzoneConst.ETAG constant and replace "ETag" occurr…

2647df0

…ences with the ref to it * replace "Md5" occurrences with the ref to OzoneConst.MD5_HASH const

HDDS-9680. fix checkstyle issues

2790d2a

HDDS-9680. revert ozone client interface proto.lock changes

1b47c00

HDDS-9680. fix unit tests issues

fd71ce1

adoroszlai reviewed Jan 29, 2024

View reviewed changes

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto Outdated Show resolved Hide resolved

HDDS-9680. makepartName of MultipartCommitUploadPartResponse as optio…

72ecefe

…nal field (revert change)

adoroszlai requested a review from kerneltime January 29, 2024 07:44

adoroszlai requested a review from ivandika3 January 29, 2024 07:44

ivandika3 reviewed Jan 29, 2024

View reviewed changes

HDDS-9680. improve an eTag property reading from the key metadata, fi…

255d7ab

…x MPU commit request computing of eTag hash for old clients

adoroszlai added 2 commits February 6, 2024 06:50

Merge remote-tracking branch 'origin/master' into HDDS-9680-multipart…

d9429a1

…-uploaded-part-eTag-improvement

Merge remote-tracking branch 'origin/master' into HDDS-9680-multipart…

6cde5b3

…-uploaded-part-eTag-improvement

kerneltime approved these changes Feb 13, 2024

View reviewed changes

adoroszlai merged commit 7370676 into apache:master Feb 13, 2024
36 checks passed

ivandika3 mentioned this pull request Apr 5, 2024

[DO NOT MERGE] Backport some fixes from master to ozone-1.4 #6479

Merged

ivandika3 pushed a commit to ivandika3/ozone that referenced this pull request Apr 18, 2024

HDDS-9680. Use md5 hash of multipart object part's content as ETag (a…

8290f86

…pache#5668) (cherry picked from commit 7370676)

ivandika3 mentioned this pull request Apr 19, 2024

[DO NOT MERGE] Backport ETag improvements and bug fixes from master to ozone-1.4 #6557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-9680. Use md5 hash of multipart object part's content as ETag #5668

HDDS-9680. Use md5 hash of multipart object part's content as ETag #5668

vtutrinov commented Nov 24, 2023 •

edited

Loading

kerneltime commented Nov 28, 2023

vtutrinov commented Nov 29, 2023

kerneltime commented Nov 29, 2023

kerneltime commented Dec 4, 2023

adoroszlai commented Dec 15, 2023

myskov commented Dec 19, 2023

kerneltime commented Jan 4, 2024

SaketaChalamchala commented Jan 5, 2024

adoroszlai commented Jan 25, 2024

vtutrinov commented Jan 25, 2024

adoroszlai commented Jan 25, 2024

kerneltime left a comment

adoroszlai commented Jan 28, 2024

vtutrinov commented Jan 29, 2024

adoroszlai commented Jan 29, 2024

ivandika3 left a comment

ivandika3 Jan 29, 2024

ivandika3 Jan 30, 2024

ivandika3 Jan 29, 2024 •

edited

Loading

ivandika3 Jan 30, 2024 •

edited

Loading

ivandika3 Jan 29, 2024

ivandika3 commented Jan 30, 2024

adoroszlai commented Jan 30, 2024

ivandika3 commented Feb 6, 2024

adoroszlai commented Feb 13, 2024

HDDS-9680. Use md5 hash of multipart object part's content as ETag #5668

HDDS-9680. Use md5 hash of multipart object part's content as ETag #5668

Conversation

vtutrinov commented Nov 24, 2023 • edited Loading

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

kerneltime commented Nov 28, 2023

vtutrinov commented Nov 29, 2023

kerneltime commented Nov 29, 2023

kerneltime commented Dec 4, 2023

adoroszlai commented Dec 15, 2023

myskov commented Dec 19, 2023

kerneltime commented Jan 4, 2024

SaketaChalamchala commented Jan 5, 2024

adoroszlai commented Jan 25, 2024

vtutrinov commented Jan 25, 2024

adoroszlai commented Jan 25, 2024

kerneltime left a comment

Choose a reason for hiding this comment

adoroszlai commented Jan 28, 2024

vtutrinov commented Jan 29, 2024

adoroszlai commented Jan 29, 2024

ivandika3 left a comment

Choose a reason for hiding this comment

ivandika3 Jan 29, 2024

Choose a reason for hiding this comment

ivandika3 Jan 30, 2024

Choose a reason for hiding this comment

ivandika3 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

ivandika3 Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

ivandika3 Jan 29, 2024

Choose a reason for hiding this comment

ivandika3 commented Jan 30, 2024

adoroszlai commented Jan 30, 2024

ivandika3 commented Feb 6, 2024

adoroszlai commented Feb 13, 2024

vtutrinov commented Nov 24, 2023 •

edited

Loading

ivandika3 Jan 29, 2024 •

edited

Loading

ivandika3 Jan 30, 2024 •

edited

Loading