Skip to content

HDDS-13477. Add OM S3 multipart commit and complete response tests#10605

Merged
adoroszlai merged 4 commits into
apache:masterfrom
chihsuan:HDDS-13477
Jun 26, 2026
Merged

HDDS-13477. Add OM S3 multipart commit and complete response tests#10605
adoroszlai merged 4 commits into
apache:masterfrom
chihsuan:HDDS-13477

Conversation

@chihsuan

@chihsuan chihsuan commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Add unit-test coverage for the non-FSO (legacy/OBS) S3 multipart commit and complete responses, which previously only had FSO tests. Following the Initiate/Abort response tests, the non-FSO test now holds the shared body and the FSO test only overrides the FSO-specific methods. The 5 changed files:

File Change Purpose
TestS3MultipartUploadCommitPartResponse New Non-FSO commit base test (missing coverage)
TestS3MultipartUploadCompleteResponse New Non-FSO complete base test (missing coverage)
TestS3MultipartUploadCommitPartResponseWithFSO Refactored Now extends the base test; keeps only FSO overrides
TestS3MultipartUploadCompleteResponseWithFSO Refactored Now extends the base test; keeps only FSO overrides
TestS3MultipartResponse Updated Adds shared non-FSO helpers createS3CommitMPUResponse / createS3CompleteMPUResponse

FSO coverage is unchanged; the two FSO tests just get much smaller.

This continues the work from the earlier PR #9206, which proposed the same approach but was auto-closed due to inactivity after review. This PR carries it forward and incorporates the review feedback left there:

  • Use the production helper OMMultipartUploadUtils.getMultipartOpenKey(..., bucketLayout) to compute the multipart open key in the complete test instead of hand-rolling the FSO/non-FSO key, which also removes a layout-specific override.
  • Assert on the committed DB state rather than the pre-commitBatchOperation state in the commit test: after commit the open key is removed and the part is persisted to the multipart info table, so it is now assertNotNull.
  • Failure-scenario coverage for completion is intentionally left out and should be a separate Jira, to keep this change focused.

What is the link to the Apache JIRA

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

How was this patch tested?

  • Added non-FSO unit tests: TestS3MultipartUploadCommitPartResponse (3 cases) and TestS3MultipartUploadCompleteResponse (4 cases).
  • Ran all S3 multipart response tests (commit/complete non-FSO and FSO, plus the unchanged initiate/abort/expired tests) locally; all pass.
  • Ran checkstyle.sh (0 violations), rat.sh, and author.sh locally.
  • FSO test timings are unchanged; the added wall-clock time is only the new non-FSO cases (per-test cost is dominated by the RocksDB OmMetadataManager setup in @BeforeEach).

@chihsuan chihsuan marked this pull request as ready for review June 25, 2026 11:44
@adoroszlai adoroszlai requested a review from ivandika3 June 25, 2026 13:23
@adoroszlai

adoroszlai commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Thanks @chihsuan for continuing work from #9206.

When merging this PR, please add Co-authored-by: Michael Chu <kousei47747@gmail.com> to reflect original efforts.

@ivandika3 ivandika3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chihsuan , overall LGTM. Left minor comments.

chihsuan added 2 commits June 26, 2026 18:16
…onse tests

Make the open key assertions meaningful: seed the part's open key before
commit so post-commit assertNull proves removal, assert the initiate MPU
open key (different key format) survives commit, and move commitOnePart
assertions after commitBatchOperation to check committed state.
Call OmMultipartKeyInfo#addPartKeyInfo directly, key the seeded prev
deleted entry with getOzoneDeletePathKey, rename deleteEntryCount to
expectedDeleteEntryCount, and pass the incremented count directly.
@chihsuan

Copy link
Copy Markdown
Contributor Author

Thanks for the review! 🙏 @ivandika3

Addressed in two commits:

@chihsuan chihsuan requested a review from ivandika3 June 26, 2026 10:31

@ivandika3 ivandika3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chihsuan for the update. LGTM +1.

@adoroszlai adoroszlai merged commit b692153 into apache:master Jun 26, 2026
32 checks passed
@adoroszlai

adoroszlai commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Thanks @chihsuan, @kousei47747 for the patch, @ivandika3, @jojochuang, @sreejasahithi for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants