errose28 commented on code in PR #10062:
URL: https://github.com/apache/ozone/pull/10062#discussion_r3456555236
##########
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()
+ throws Exception {
+ String volumeName = UUID.randomUUID().toString();
+ String bucketName = UUID.randomUUID().toString();
+ String keyName = getKeyName();
+
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName,
+ omMetadataManager, getBucketLayout());
+
+ createParentPath(volumeName, bucketName);
+
+ String multipartUploadID =
+ initiateMultipartUploadWithSchemaVersion(volumeName, bucketName,
+ keyName, (byte) 1);
+
+ String multipartKey = omMetadataManager.getMultipartKey(volumeName,
+ bucketName, keyName, multipartUploadID);
+ OmMultipartKeyInfo multipartKeyInfo = omMetadataManager
+ .getMultipartInfoTable().get(multipartKey);
+ assertNotNull(multipartKeyInfo);
+ assertEquals(1, multipartKeyInfo.getSchemaVersion());
+
+ OMRequest abortMPURequest =
+ doPreExecuteAbortMPU(volumeName, bucketName, keyName,
+ multipartUploadID);
+
+ S3MultipartUploadAbortRequest s3MultipartUploadAbortRequest =
+ getS3MultipartUploadAbortReq(abortMPURequest);
+
+ OMClientResponse omClientResponse =
+ s3MultipartUploadAbortRequest.validateAndUpdateCache(ozoneManager, 2L);
+
+ assertNotEquals(OzoneManagerProtocolProtos.Status
+ .NOT_SUPPORTED_OPERATION_PRIOR_FINALIZATION,
+ omClientResponse.getOMResponse().getStatus());
+ assertEquals(OzoneManagerProtocolProtos.Status.OK,
+ omClientResponse.getOMResponse().getStatus());
Review Comment:
The status can only have one value, so the prior `assertNotEquals` call is
redundant given the `assertEquals` following it. The status also jumps out
visually more than the `Not` in the method name so removing the other assertion
makes this easier when skimming.
There's a similar pattern in other tests added as well.
```suggestion
assertEquals(OzoneManagerProtocolProtos.Status.OK,
omClientResponse.getOMResponse().getStatus());
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3InitiateMultipartUploadRequest.java:
##########
@@ -72,6 +72,7 @@ public class S3InitiateMultipartUploadRequest extends
OMKeyRequest {
private static final Logger LOG =
LoggerFactory.getLogger(S3InitiateMultipartUploadRequest.class);
+ private static final int LEGACY_MPU_SCHEMA_VERSION = 0;
Review Comment:
Can we define the schema versions in one place as public constants? Right
now definitions are duplicated between this class and `OmMultipartKeyInfo`.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3MultipartUploadCommitPartRequestWithFSO.java:
##########
@@ -71,6 +76,31 @@ protected String getKeyName() {
return dirName + UUID.randomUUID().toString();
}
+ @Test
+ public void testValidateAndUpdateCacheDirectoryNotFound() throws Exception {
Review Comment:
Is this test related to the MPU version somehow?
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3MultipartRequest.java:
##########
@@ -302,6 +306,46 @@ protected OMRequest doPreExecuteCompleteMPU(
}
+ /**
+ * Initiate an MPU and optionally rewrite the stored multipart metadata to a
+ * specific schema version.
+ *
+ * <p>The schema version rewrite lets tests emulate post-finalization MPU
+ * entries without needing the rest of the upgrade pipeline.</p>
+ */
+ protected String initiateMultipartUploadWithSchemaVersion(
+ String volumeName, String bucketName, String keyName,
+ byte schemaVersion) throws Exception {
Review Comment:
nit. `schemaVersion` is an int now.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3MultipartUploadCommitPartRequest.java:
##########
@@ -151,6 +248,10 @@ public void testValidateAndUpdateCacheMultipartNotFound()
throws Exception {
// Add key to open key table.
addKeyToOpenKeyTable(volumeName, bucketName, keyName, clientID);
+ String openKey = getOpenKey(volumeName, bucketName, keyName, clientID);
Review Comment:
Why were these last parts after the new test added? They don't seem directly
related to the feature.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3InitiateMultipartUploadRequest.java:
##########
@@ -284,6 +291,35 @@ protected void logResult(OzoneManager ozoneManager,
}
}
+ protected int resolveMultipartSchemaVersion(
+ MultipartInfoInitiateRequest multipartInfoInitiateRequest) {
+ if (!multipartInfoInitiateRequest.hasSchemaVersion()) {
+ return LEGACY_MPU_SCHEMA_VERSION;
+ }
+ return (int) multipartInfoInitiateRequest.getSchemaVersion();
+ }
+
+ @RequestFeatureValidator(
+ conditions = {
+ ValidationCondition.CLUSTER_NEEDS_FINALIZATION,
+ ValidationCondition.CLUSTER_HAS_MPU_PARTS_TABLE_SPLIT
+ },
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.InitiateMultiPartUpload
+ )
+ public static OMRequest setSchemaVersionOnInitiateMultipartUpload(
+ OMRequest req, ValidationContext ctx) {
+ MultipartInfoInitiateRequest initiateRequest =
+ req.getInitiateMultiPartUploadRequest();
+
+ // Keep newly initiated MPUs on legacy schema until split parts-table
+ // write/read paths are fully implemented.
+ return req.toBuilder()
+ .setInitiateMultiPartUploadRequest(initiateRequest.toBuilder()
+ .setSchemaVersion(LEGACY_MPU_SCHEMA_VERSION))
Review Comment:
I may have missed this in an earlier comment, but this change has no effect
if we commit it with the new layout version but a no-op action attached to
that. Say a release goes out with this PR as is. The new MPU version will get
finalized, but nothing changes. In the next release we would then need to add a
second MPU layout version to check because the first one was basically wasted.
The HBase feature made this mistake while trying to develop on master, that's
why there's a "deprecated" HBase layout feature and the actual one comes later.
So if we merge this in its current state the master branch is unreleasable.
We either need to hold off on this change and leave the new schema hardcoded to
off until it is ready for release, or decide that work is complete and flip the
switch to enable it for finalized clusters in this PR.
##########
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:
It's somewhat unintuitive how the parent class sets the version manager to
pre-finalized by default and tests need to opt in to finalize it. If it's too
much change to reverse that in this PR, let's at least add a comment about it
on these tests.
--
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]
