Skip to content

Conversation

@unknowntpo
Copy link
Contributor

@unknowntpo unknowntpo commented Oct 6, 2025

Extract duplicate code for processing keys and acquiring locks into a helper method. Both subdirectory and subfile processing loops had identical code for:

  • Converting protobuf key to OmKeyInfo
  • Creating path and delete keys
  • Creating volBucketPair

Introduced ProcessedKeyInfo helper class to encapsulate all extracted information (keyInfo, deleteKey, volumeName, bucketName, volBucketPair) and processDeleteKey method to eliminate code duplication.

What changes were proposed in this pull request?

Extract duplicated code.

What is the link to the Apache JIRA

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

How was this patch tested?

UT: TestOMDirectoriesPurgeRequestAndResponse

@unknowntpo unknowntpo force-pushed the HDDS-13722-dup-code-in-OMDirectoriesPurgeRequestWithFSO branch from 1b674c6 to 216f57a Compare October 6, 2025 11:26
@unknowntpo unknowntpo marked this pull request as ready for review October 6, 2025 11:29
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @unknowntpo for the patch. Please enable the build-branch workflow in your fork.

Comment on lines 243 to 249
@SuppressWarnings("visibilitymodifier")
private static class ProcessedKeyInfo {
final OmKeyInfo keyInfo;
final String deleteKey;
final String volumeName;
final String bucketName;
final Pair<String, String> volBucketPair;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suppression doesn't seem to be necessary.

Suggested change
@SuppressWarnings("visibilitymodifier")
private static class ProcessedKeyInfo {
final OmKeyInfo keyInfo;
final String deleteKey;
final String volumeName;
final String bucketName;
final Pair<String, String> volBucketPair;
private static class ProcessedKeyInfo {
private final OmKeyInfo keyInfo;
private final String deleteKey;
private final String volumeName;
private final String bucketName;
private final Pair<String, String> volBucketPair;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, build-branch is enabled. And suggestion is applied.

@jojochuang jojochuang requested a review from Copilot October 7, 2025 05:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors duplicate code in OMDirectoriesPurgeRequestWithFSO by extracting common key processing logic into a reusable helper method. The changes eliminate code duplication between subdirectory and subfile processing loops while maintaining the same functionality.

  • Extracted common key processing logic into processDeleteKey helper method
  • Introduced ProcessedKeyInfo helper class to encapsulate processed key information
  • Replaced duplicate code blocks in both subdirectory and subfile processing loops

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +243 to +248
private static class ProcessedKeyInfo {
private final OmKeyInfo keyInfo;
private final String deleteKey;
private final String volumeName;
private final String bucketName;
private final Pair<String, String> volBucketPair;
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

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

The ProcessedKeyInfo class exposes all fields as package-private, but they should be private with getter methods for better encapsulation. Direct field access breaks encapsulation principles.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getter is too verbose.

Comment on lines 250 to 251
ProcessedKeyInfo(OmKeyInfo keyInfo, String deleteKey, String volumeName, String bucketName,
Pair<String, String> volBucketPair) {
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

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

The constructor accepts both individual volumeName/bucketName parameters and a volBucketPair containing the same information, creating redundancy. Consider creating the volBucketPair internally from volumeName and bucketName to eliminate this duplication.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, fixed at 05ffdc8

@unknowntpo unknowntpo force-pushed the HDDS-13722-dup-code-in-OMDirectoriesPurgeRequestWithFSO branch from 9d0e607 to 05ffdc8 Compare October 7, 2025 07:58
@jojochuang
Copy link
Contributor

+1

@jojochuang jojochuang merged commit 2ae1f65 into apache:master Oct 7, 2025
43 checks passed
@jojochuang
Copy link
Contributor

Merged. Thanks @unknowntpo for contribution and @adoroszlai for review.

@unknowntpo unknowntpo deleted the HDDS-13722-dup-code-in-OMDirectoriesPurgeRequestWithFSO branch October 13, 2025 03:43
swamirishi pushed a commit to swamirishi/ozone that referenced this pull request Dec 3, 2025
…RequestWithFSO (apache#9102)

(cherry picked from commit 2ae1f65)

 Conflicts:
	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java

Change-Id: I02c7d85a3b21fbcfce52e1708b769d1fd8bd8841
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Jan 12, 2026
…RequestWithFSO (apache#9102)

Change-Id: Ia129a097f64815d38eea725733db2aca6e4cec77
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.

3 participants