errose28 commented on code in PR #10062:
URL: https://github.com/apache/ozone/pull/10062#discussion_r3626558142
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmMultipartPartInfo.java:
##########
@@ -70,9 +69,10 @@ private OmMultipartPartInfo(Builder b) {
if (b.partNumber <= 0) {
throw new IllegalArgumentException("partNumber is required and > 0");
}
- if (StringUtils.isBlank(b.eTag)) {
- throw new IllegalArgumentException("eTag is required");
- }
+ // eTag is optional: not all MPU clients supply an ETag at commit time
+ // (e.g. the Ozone native client), matching the legacy inline flow which
+ // never required it. It is stored when present and used for validation
+ // during CompleteMultipartUpload only when the client provides one.
Review Comment:
My understanding is that for the old path etags were not enforced on the OM
server side, and on the new path they were, and we want to make both paths
consistent. The PR currently chose to make both match the old path with no OM
server enforcement, but this makes it possible for a native client to create
MPUs with no Etag which will appear as corrupted to S3 clients. Actually the OM
server side enforcement for the new path was correct and that should be added
in the old path as well.
The original tests are wrong and easy to update since the etag just needs to
be non-empty to pass the original validation. We can do this in a separate
Jira, but it should land before this one to keep the master branch correct. I'm
also ok with putting combining it in this PR though. I just don't want the
enforcement removal merged into master. Probably something like this would
suffice for the tests:
```diff
diff --git
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotTests.java
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotTests.java
index 44131d81c8..27fcd764f1 100644
---
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotTests.java
+++
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotTests.java
@@ -129,6 +129,7 @@
import org.apache.hadoop.ozone.client.OzoneSnapshot;
import org.apache.hadoop.ozone.client.OzoneSnapshotDiff;
import org.apache.hadoop.ozone.client.OzoneVolume;
+import org.apache.hadoop.ozone.client.io.KeyMetadataAware;
import org.apache.hadoop.ozone.client.io.OzoneDataStreamOutput;
import org.apache.hadoop.ozone.client.io.OzoneInputStream;
import org.apache.hadoop.ozone.client.io.OzoneOutputStream;
@@ -3261,18 +3262,21 @@ public void
testSnapshotDiffWithCreateMultipartKeys() throws Exception {
try (OzoneOutputStream stream = bucket.createMultipartKey(
regularPartsKey, regularPart.length, 1,
regularMpuInfo.getUploadID())) {
stream.write(regularPart);
+ setPartETag(stream);
}
byte[] streamPart = "stream data".getBytes(UTF_8);
try (OzoneDataStreamOutput streamOut = bucket.createMultipartStreamKey(
streamPartsKey, streamPart.length, 1, streamMpuInfo.getUploadID()))
{
streamOut.write(streamPart);
+ setPartETag(streamOut);
}
byte[] mixedPart = "mixed data".getBytes(UTF_8);
try (OzoneOutputStream mixedStream = bucket.createMultipartKey(
mixedPartsKey, mixedPart.length, 1, mixedMpuInfo.getUploadID())) {
mixedStream.write(mixedPart);
+ setPartETag(mixedStream);
}
assertEquals(1,
@@ -3322,14 +3326,17 @@ public void
testSnapshotDiffWithAbortMultipartUpload() throws Exception {
try (OzoneOutputStream part1Stream = bucket.createMultipartKey(
partialAbortKey, part1Data.length, 1, partialInfo.getUploadID())) {
part1Stream.write(part1Data);
+ setPartETag(part1Stream);
}
try (OzoneOutputStream part2Stream = bucket.createMultipartKey(
partialAbortKey, part2Data.length, 2, partialInfo.getUploadID())) {
part2Stream.write(part2Data);
+ setPartETag(part2Stream);
}
try (OzoneDataStreamOutput part3Stream =
bucket.createMultipartStreamKey(
partialAbortKey, part3Data.length, 3, partialInfo.getUploadID())) {
part3Stream.write(part3Data);
+ setPartETag(part3Stream);
}
OzoneMultipartUploadPartListParts partsList = bucket.listParts(
@@ -3348,10 +3355,12 @@ public void
testSnapshotDiffWithAbortMultipartUpload() throws Exception {
try (OzoneOutputStream stream = bucket.createMultipartKey(
multiAbortKey1, part1Data.length, 1, multiInfo1.getUploadID())) {
stream.write(part1Data);
+ setPartETag(stream);
}
try (OzoneDataStreamOutput stream = bucket.createMultipartStreamKey(
multiAbortKey2, part2Data.length, 1, multiInfo2.getUploadID())) {
stream.write(part2Data);
+ setPartETag(stream);
}
bucket.abortMultipartUpload(multiAbortKey1, multiInfo1.getUploadID());
@@ -3398,10 +3407,12 @@ public void
testSnapshotDiffWithCompleteInvisibleMPULifecycle() throws Exception
try (OzoneOutputStream stream = bucket.createMultipartKey(
mpuKey1, regularData1.length, 1, mpuInfo1.getUploadID())) {
stream.write(regularData1);
+ setPartETag(stream);
}
try (OzoneOutputStream stream = bucket.createMultipartKey(
mpuKey1, regularData2.length, 2, mpuInfo1.getUploadID())) {
stream.write(regularData2);
+ setPartETag(stream);
}
byte[] streamData1 = "Stream multipart data 1".getBytes(UTF_8);
@@ -3410,10 +3421,12 @@ public void
testSnapshotDiffWithCompleteInvisibleMPULifecycle() throws Exception
try (OzoneDataStreamOutput stream = bucket.createMultipartStreamKey(
mpuKey2, streamData1.length, 1, mpuInfo2.getUploadID())) {
stream.write(streamData1);
+ setPartETag(stream);
}
try (OzoneDataStreamOutput stream = bucket.createMultipartStreamKey(
mpuKey2, streamData2.length, 2, mpuInfo2.getUploadID())) {
stream.write(streamData2);
+ setPartETag(stream);
}
@@ -3423,10 +3436,12 @@ public void
testSnapshotDiffWithCompleteInvisibleMPULifecycle() throws Exception
try (OzoneOutputStream stream = bucket.createMultipartKey(
mpuKey3, mixedRegular.length, 1, mpuInfo3.getUploadID())) {
stream.write(mixedRegular);
+ setPartETag(stream);
}
try (OzoneDataStreamOutput stream = bucket.createMultipartStreamKey(
mpuKey3, mixedStream.length, 2, mpuInfo3.getUploadID())) {
stream.write(mixedStream);
+ setPartETag(stream);
}
assertEquals(2,
@@ -3470,6 +3485,7 @@ private void completeSinglePartMPU(OzoneBucket bucket,
String keyName, String da
byte[] partData = createLargePartData(data, MIN_PART_SIZE);
OzoneOutputStream partStream = bucket.createMultipartKey(keyName,
partData.length, 1, uploadId);
partStream.write(partData);
+ setPartETag(partStream);
partStream.close();
OzoneMultipartUploadPartListParts partsList = bucket.listParts(keyName,
uploadId, 0, 100);
@@ -3491,6 +3507,7 @@ private void completeMultiplePartMPU(
try (OzoneOutputStream partStream = bucket.createMultipartKey(
keyName, partData.length, partNum, uploadId)) {
partStream.write(partData);
+ setPartETag(partStream);
}
}
@@ -3514,12 +3531,14 @@ private void completeMixedPartMPU(
try (OzoneOutputStream partStream = bucket.createMultipartKey(
keyName, part1Data.length, 1, uploadId)) {
partStream.write(part1Data);
+ setPartETag(partStream);
}
byte[] part2Data = createLargePartData(streamData, MIN_PART_SIZE);
try (OzoneDataStreamOutput partStream = bucket.createMultipartStreamKey(
keyName, part2Data.length, 2, uploadId)) {
partStream.write(part2Data);
+ setPartETag(partStream);
}
OzoneMultipartUploadPartListParts partsList = bucket.listParts(keyName,
uploadId, 0, 2);
@@ -3547,6 +3566,7 @@ private void completeMPUWithReplication(
try (OzoneOutputStream partStream = bucket.createMultipartKey(
keyName, partData.length, 1, uploadId)) {
partStream.write(partData);
+ setPartETag(partStream);
}
OzoneMultipartUploadPartListParts partsList = bucket.listParts(keyName,
uploadId, 0, 1);
@@ -3566,6 +3586,7 @@ private void completeMPUWithMetadata(OzoneBucket
bucket, String keyName,
byte[] partData = createLargePartData("MPU with metadata and tags",
MIN_PART_SIZE);
OzoneOutputStream partStream = bucket.createMultipartKey(keyName,
partData.length, 1, uploadId);
partStream.write(partData);
+ setPartETag(partStream);
partStream.close();
OzoneMultipartUploadPartListParts partsList = bucket.listParts(keyName,
uploadId, 0, 100);
@@ -3596,6 +3617,10 @@ private byte[] createLargePartData(String
baseContent, int targetSize) {
return result.getBytes(UTF_8);
}
+ private static void setPartETag(KeyMetadataAware stream) {
+ stream.getMetadata().put(OzoneConsts.ETAG,
UUID.randomUUID().toString());
+ }
+
@Test
public void testSnapshotDiffMPUCreateNewKey() throws Exception {
String testVolumeName = "vol-create-new-" + counter.incrementAndGet();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
