Skip to content

Refactor duplicated S3 object tagging request code into shared base class#4

Open
devin-ai-integration[bot] wants to merge 1 commit into
masterfrom
devin/1782584341-refactor-s3-tagging-dedup
Open

Refactor duplicated S3 object tagging request code into shared base class#4
devin-ai-integration[bot] wants to merge 1 commit into
masterfrom
devin/1782584341-refactor-s3-tagging-dedup

Conversation

@devin-ai-integration

Copy link
Copy Markdown

What changes were proposed in this pull request?

This PR eliminates duplicated code in the OM S3 object tagging request handlers and removes a copy-pasted method in OMSnapshotCreateRequest.

S3 Object Tagging — extract S3ObjectTaggingRequest abstract base

S3PutObjectTaggingRequest and S3DeleteObjectTaggingRequest (plus their FSO variants) had ~90% identical preExecute and validateAndUpdateCache implementations. The only differences were:

  • Tag mutation: setTags(KeyValueUtil.getFromProtobuf(...)) vs setTags(Collections.emptyMap())
  • Metrics: incNumPutObjectTagging vs incNumDeleteObjectTagging
  • Audit action and log messages
  • Proto request/response field names

A new abstract S3ObjectTaggingRequest base class now provides the common preExecute, OBS validateAndUpdateCache, and FSO validateAndUpdateCacheFSO template methods. Concrete subclasses supply only the operation-specific hooks via abstract methods:

// Before: each class had ~190 lines of near-identical code
// After: concrete classes are ~110 lines of hooks only

protected abstract KeyArgs getTaggingKeyArgs(OMRequest request);
protected abstract Map<String, String> getUpdatedTags(KeyArgs keyArgs);
protected abstract OMResponse.Builder setTaggingResponse(OMResponse.Builder b);
protected abstract void incrementMetrics(OMMetrics metrics);
protected abstract AuditAction getAuditAction();
// ... etc

The FSO variants (S3PutObjectTaggingRequestWithFSO, S3DeleteObjectTaggingRequestWithFSO) are now reduced to a single validateAndUpdateCache override that delegates to validateAndUpdateCacheFSO.

OMSnapshotCreateRequest.getBucketInfoOMKeyRequest.getBucketInfo

The method was explicitly documented as "Same as OMKeyRequest#getBucketInfo" and was a copy-paste. Replaced with a call to the existing static method OMKeyRequest.getBucketInfo(...).

Net effect: ~105 fewer lines of duplicated code across 6 files.

Generated-by: Devin (Claude)

What is the link to the Apache JIRA

N/A — this is a refactoring-only change on a personal fork.

How was this patch tested?

  • All 8 existing test classes pass (27 tests total): TestS3PutObjectTaggingRequest, TestS3DeleteObjectTaggingRequest, TestS3PutObjectTaggingRequestWithFSO, TestS3DeleteObjectTaggingRequestWithFSO, TestS3PutObjectTaggingResponse, TestS3DeleteObjectTaggingResponse, TestS3PutObjectTaggingResponseWithFSO, TestS3DeleteObjectTaggingResponseWithFSO
  • TestOMSnapshotCreateRequest passes (18 tests)
  • checkstyle.sh: 0 violations
  • rat.sh: passes (license headers on new file verified)
  • Full module build: mvn -pl :ozone-manager -am install -DskipTests -DskipShade -DskipRecon -DskipDocs succeeds

Link to Devin session: https://app.devin.ai/sessions/874da879b185407abe9f9f882e95865d
Requested by: @marcuslin123

… OMSnapshotCreateRequest.getBucketInfo duplication

- Extract S3ObjectTaggingRequest abstract base class consolidating
  ~90% duplicated code between S3PutObjectTaggingRequest and
  S3DeleteObjectTaggingRequest (both OBS and FSO variants)
- Replace OMSnapshotCreateRequest.getBucketInfo copy-paste with
  OMKeyRequest.getBucketInfo static call

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@marcuslin123 marcuslin123 self-assigned this Jun 27, 2026
@devin-ai-integration

Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant