HDDS-14665. Add upgrade handling to multipart requests

[spacemonkd]

What changes were proposed in this pull request?

HDDS-14665. Add upgrade handling to multipart requests

Please describe your PR in detail:

• This PR adds a new layout version to define the split table behaviour for Multipart Uploads
• It also adds new checks which fails early in case the finalization is not done and the writes try to use the new split table

What is the link to the Apache JIRA

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

How was this patch tested?

Patch was tested using unit tests

[kerneltime]

Review

Thanks for landing the upgrade gate ahead of the parts-table-split write path — gating first is the right sequencing. The layout-feature wiring looks correct (no upgrade action / MLV constant needed for a behavior-only feature, and TestOMVersionManager's contiguity check still passes), and the gate's pre-finalization determinism holds: MLV is advanced only by the Ratis-logged OMFinalizeUpgradeRequest, so isAllowed resolves identically across replicas at a given apply index.

I think there's one blocker, and it's not in the gate itself — it's in the CommitPart null-check reorder that came along with it.

Blocker — CommitPart: moving the multipartKeyInfo == null check above getOmKeyInfo() crashes the OM on the commit-after-abort path

To read multipartKeyInfo.getSchemaVersion() safely, the multipartKeyInfo == null -> NO_SUCH_MULTIPART_UPLOAD_ERROR check was moved up to right after getMultipartInfoTable().get(), ahead of omKeyInfo = getOmKeyInfo(...). But the NO_SUCH path is special: S3MultipartUploadCommitPartResponse.checkAndUpdateDB() overrides the flush to GC the orphaned open part and unconditionally dereferences openPartKeyInfoToBeDeleted (the omKeyInfo handed in by the request) — S3MultipartUploadCommitPartResponse.java:88-97 (.getUpdateID(), .getObjectID()).

• On master, omKeyInfo is loaded and null-checked (-> KEY_NOT_FOUND) before the NO_SUCH throw, so a NO_SUCH response always carried a non-null part to delete.
• With this PR, NO_SUCH is thrown while omKeyInfo is still null; the catch passes that null into the response. Error responses are enqueued unconditionally (RequestHandler.handleWriteRequest), and OzoneManagerDoubleBuffer calls checkAndUpdateDB for them and terminate()s the OM on any Throwable. So the flush NPEs -> ExitUtils.terminate -> OM exits, and followers re-applying the same entry exit identically.

This is exactly the scenario the GC branch exists for — a part commit arriving after the upload was aborted. testValidateAndUpdateCacheMultipartNotFound already sets up the precondition (random uploadId + addKeyToOpenKeyTable); it stays green only because it asserts the response status and never flushes the double buffer, so checkAndUpdateDB is never run in the unit test. It also affects legacy (schemaVersion 0) uploads, so it's independent of the new feature.

Suggested fix: leave the multipartKeyInfo == null check at its original position (after omKeyInfo is resolved) and put the new gate immediately after it — the gate only needs multipartKeyInfo non-null, which the original ordering already guaranteed there. A regression test that actually flushes the double buffer (or a MiniOzoneCluster commit-after-abort case) would lock it down.

Concerns

• The allowed (post-finalization) branch is never tested. The base test harness mocks OMLayoutVersionManager, so isAllowed(MPU_PARTS_TABLE_SPLIT) is always false (and MPU_PARTS_TABLE_SPLIT appears in no test). The predicate !isAllowed && schemaVersion != 0 is only ever driven from the left operand — a future inversion (dropped !, wrong feature) would still pass CI. Please add an isAllowed -> true case per request (Abort can assert full success; for Commit/Complete at least assertNotEquals(NOT_SUPPORTED_OPERATION_PRIOR_FINALIZATION, ...)).
• Unrelated change bundled in. The existingKeyInfo swap to getOmKeyInfoFromKeyTable(...) in S3MultipartUploadCompleteRequest is behaviorally inert today (base impl equals the old getKeyTable(...).get(dbOzoneKey); the FSO override only adds setKeyName, which validateAtomicRewrite/validateIfMatchETag don't read) and is unrelated to finalization — and line 333 still uses the raw lookup for the same key. Suggest dropping it here or splitting into its own JIRA.
• In-apply isAllowed read departs from the established pattern. Every other layout gate — including the EC CLUSTER_NEEDS_FINALIZATION validators in these same two files — runs as a leader-only PRE_PROCESS @RequestFeatureValidator. This is the first to read isAllowed inside the replicated validateAndUpdateCache path, which the method's Javadoc cautions against. It's safe today (MLV advances synchronously via the logged finalize), but that relies on the finalization executor staying synchronous (it's pluggable). Worth considering gating on the request-stamped LayoutVersion from preExecute, or at least a comment documenting the determinism assumption.
• Dropped FSO coverage. Rewriting testValidateAndUpdateCacheKeyNotFound to initiate a real MPU removes the FSO DIRECTORY_NOT_FOUND branch the old assertion covered; that path is now unasserted in the commit-part suite. Could be restored as a small separate test.

Probably for the write-side PR (flagging so they aren't silent regressions later)

Not reachable until a sibling PR writes schemaVersion != 0, so not blockers here:

• KeyManagerImpl.listParts reads the inline getPartKeyInfoMap() (empty for split entries) -> would return 200 with zero parts.
• The background S3ExpiredMultipartUploadsAbortRequest isn't gated and derives quota from the empty inline map -> on a downgraded cluster it would delete the row releasing 0 quota while user-facing Abort is gated off.
• NOT_SUPPORTED_OPERATION_PRIOR_FINALIZATION has no case in S3ErrorTable.translateResultCode -> falls to default -> HTTP 500 to S3 clients.

Nits

• Complete gate message reads "...for commit part request." — should be the complete path.
• Double blank line after the gate block in S3MultipartUploadAbortRequest.
• schemaVersion is uint32 on the wire but narrowed to byte on load; gate tests != 0 while the model special-cases == 1. Not reachable today, but a multiple-of-256 value would truncate to 0 (fail-open) — worth validating to {0,1} at the boundary.

[spacemonkd]

@kerneltime @ivandika3 it would be great if you could take a look

[errose28]

Thanks for working on this. Not sure if you've had a chance to address the AI comments above as well.

Looks like nothing is setting schema version right now which makes it difficult to review this end to end. Can we add that to this PR?

[amaliujia]

Any reason why we still keep SchemaVersion as a byte in JVM? In its proto definition schemaVersion is optional uint32 schemaVersion = 10;.

[spacemonkd]

Yes, I have reverted to int.
Initially I kept as byte since we only support the values as 0 or 1 so we do not need to allocate memory for an integer, but I guess it is easier to read with it being int.

[amaliujia]

:-) for only 0 or 1 we can go for one bit.

[spacemonkd]

Thanks for the reviews @errose28, @kerneltime and @amaliujia.

Looks like nothing is setting schema version right now which makes it difficult to review this end to end.

I can add the implementation to this PR itself, but then the code changes on this would increase.
I plan on adding the last PR to this chain after this one which will introduce the implementation and usage of the schemaVersion.
Would that be okay? If not I can merge those changes as part of this PR @errose28

Could you take another look at the new changes? CI is green.

[errose28]

My understanding is that schema version to use will be assigned by the leader when the request is received based on whether or not the feature is finalized. Since finalization goes through ratis we would not need the request blocking parts of this change. The request processing would just follow whatever schema version it is given.

I do think it would be helpful to have this whole flow in one PR, even if it increases the size.

[spacemonkd]

@errose28 I have added the schemaVersion wiring as well.
Could you take another look?

Also:

if (splitPartsFeatureAllowed
          && multipartKeyInfo.getSchemaVersion() == 1) {
        throw new OMException("MPU parts-table split commit path is not " +
            "supported in this write flow.",
            OMException.ResultCodes.NOT_SUPPORTED_OPERATION);
      }

added this block so that the current builds for Ozone doesn't give error as we do not have the implementation for the actual split writing of the table. I will remove this in the follow up PR to this which will introduce the wiring.

I have pinned the schemaVersion to 0 otherwise it was giving errors with implementation missing, but the overall helpers and checks are in place to give a better idea of the end to end flow

@amaliujia could you please take a look as well?

[amaliujia]

Overall LGTM

[errose28]

Thanks for the update @spacemonkd. We should only be checking MLV to set the schema when the request is first initiated and before it is submitted to Ratis. All subsequent requests should then use the schema version in the proto metadata as the source of truth. This can be done in a pre-processor because it only needs to mutate the incoming proto's schema version based on the current MLV, then all future processing will obey that schema version.

Finalization goes through Ratis so we don't need to worry about MLV diverging between OMs.

[spacemonkd]

@errose28 are the latest changes now in line with what you meant?
I changed it to have a new check for MPU_SPLIT_TABLE allowed.
Also note that in the new pre-processor check setSchemaVersionOnInitiateMultipartUpload runs on both before and after finalize.

This constant 0 will later on be switched based on the condition whether split table is allowed or not - once we have the implementation ready in the next PR.

[ivandika3]

LGTM +1.

[spacemonkd]

@errose28 could you take a look as well?

[errose28]

Thanks for the updates. This method of choosing schema version works. I'm reworking the pre-process validators on the ZDU branch but this implementation will be easy to move. Mostly minor comments but the patch does have one blocking issue noted here.

[errose28]

Can we define the schema versions in one place as public constants? Right now definitions are duplicated between this class and OmMultipartKeyInfo.

[spacemonkd]

Addressed in the latest commit

[errose28]

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.

[spacemonkd]

Hmm, I raised #10588 to implement the split table writes.
Maybe we can merge this PR after that to avoid this situation?

Could you take a look at that PR then, when you get the time?

[errose28]

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.

[spacemonkd]

Addressed in latest commit

[errose28]

Why were these last parts after the new test added? They don't seem directly related to the feature.

[spacemonkd]

Addressed in latest commit

[errose28]

Is this test related to the MPU version somehow?