errose28 commented on code in PR #10062:
URL: https://github.com/apache/ozone/pull/10062#discussion_r3617679947
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3InitiateMultipartUploadRequest.java:
##########
@@ -113,6 +117,158 @@ public void testValidateAndUpdateCache() throws Exception
{
}
+ @Test
+ public void
testValidateAndUpdateCacheSetsSchemaVersionZeroBeforeFinalization()
+ throws Exception {
+ String volumeName = UUID.randomUUID().toString();
+ String bucketName = UUID.randomUUID().toString();
+ String keyName = UUID.randomUUID().toString();
+
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName,
+ omMetadataManager, getBucketLayout());
+
+ OMRequest modifiedRequest = doPreExecuteInitiateMPU(volumeName,
Review Comment:
We should also check the schema version here to make sure it is being set in
`preExecute` and not apply. Along those lines the title of this test is
somewhat misleading as the value is set in `preExecute` and
`validateAndUpdateCache` only forwards it. Also true for most of the following
request unit tests.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3InitiateMultipartUploadRequest.java:
##########
@@ -113,6 +117,158 @@ public void testValidateAndUpdateCache() throws Exception
{
}
+ @Test
+ public void
testValidateAndUpdateCacheSetsSchemaVersionZeroBeforeFinalization()
Review Comment:
Can we split these four tests into two pairs parameterized by finalized
(true/false) and the corresponding expected schema version? This looks doable
for most of the new request unit tests added.
##########
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:
Why is this making etags optional? I see we are keeping them required at the
gateway to be consistent with AWS but I don't understand why this change is
necessary in general or in the context of this PR.
> It is stored when present and used for validation
// during CompleteMultipartUpload only when the client provides one.
It should also be returned when the parts for an in-progress upload are
listed by the client.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3MultipartUploadAbortRequest.java:
##########
@@ -95,6 +99,95 @@ public void testValidateAndUpdateCache() throws Exception {
}
+ @Test
+ public void
testValidateAndUpdateCacheUsesSchemaVersionOneBeforeFinalization()
Review Comment:
> // Tests must explicitly bump metadata layout version to simulate
finalized OM.
Comment is fine for now, but lets also file a follow-up jira to change it.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3MultipartUploadCommitPartRequest.java:
##########
@@ -139,6 +140,97 @@ public void testValidateAndUpdateCacheSuccess() throws
Exception {
.get(partKey));
}
+ @Test
+ public void
testValidateAndUpdateCacheUsesSchemaVersionOneBeforeFinalization()
+ throws Exception {
+ String volumeName = UUID.randomUUID().toString();
+ String bucketName = UUID.randomUUID().toString();
+ String keyName = getKeyName();
+
+ // The base test fixture is pre-finalized by default.
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName,
+ omMetadataManager, getBucketLayout());
+
+ createParentPath(volumeName, bucketName);
+
+ String multipartUploadID =
+ initiateMultipartUploadWithSchemaVersion(volumeName, bucketName,
+ keyName, 1);
Review Comment:
nit. Let's use the the constant for schema versions.
--
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]
