ozone icon indicating copy to clipboard operation
ozone copied to clipboard

HDDS-13532. Refactor BucketEndpoint.get() method

Open rich7420 opened this issue 3 months ago • 6 comments

What changes were proposed in this pull request?

This PR refactors the BucketEndpoint.get() method to improve code maintainability and readability. The original method was nearly 200 lines long and contained multiple unrelated logic blocks, making it difficult to understand, test, and maintain.

Changes in this PRr

  • Extracted 5 methods from the get() method
  • add BucketListingContext class to encapsulate parameters
  • added test coverage (unit, integration, performance)

What is the link to the Apache JIRA

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

How was this patch tested?

  • Unit tests: TestBucketEndpointRefactoredSimple.java
  • Integration tests: TestBucketEndpointIntegration.java
  • Performance tests: TestBucketEndpointPerformance.java

rich7420 avatar Oct 07 '25 07:10 rich7420

Thanks, @rich7420, for the patch. Please move the integration tests to the integration module instead of keeping them with the unit tests. Also, it would be helpful if you could add some context explaining the purpose of these test cases.

priyeshkaratha avatar Oct 07 '25 09:10 priyeshkaratha

@peterxcli , @priyeshkaratha Thanks for the review! The three new tests were added mainly to verify the refactored logic in BucketEndpoint.get(). I think that they help ensure that behavior remains consistent before and after the refactor. That said, I can move the integration test to the integration module as suggested and update the comments to clarify the purpose of each test. If you prefer, I can also remove the tests after verifying it locally.

rich7420 avatar Oct 07 '25 11:10 rich7420

I’ve enabled the build-branch workflow on my fork. https://github.com/rich7420/ozone/actions/runs/18313906976/job/52148987227 Regarding the three tests: I trimmed the change set to keep the PR focused on the refactor. The newly added integration/performance tests have been removed to keep CI green and to avoid duplicating coverage that already exists in the integration suites. If the team prefers, I can reintroduce them in a follow‑up PR.

rich7420 avatar Oct 07 '25 13:10 rich7420

@jojochuang @hevinhsu @TaiJuWu Thanks a lot for the suggestions! I’ve implemented the updates that switched tests to use EndpointBuilder for endpoint creation, moved the key-listing logic into BucketListingContext with BucketEndpoint.processKeyListing(...) delegating to it, renamed the method to handleGetBucketAcl(...), added throws IOException to the test helper getObjectEndpoint(...), and fixed the trailing newline and related style checks. CI is all green now. If there’s anything else we can refine, I’m happy to follow up.

rich7420 avatar Oct 13 '25 13:10 rich7420

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

github-actions[bot] avatar Dec 03 '25 00:12 github-actions[bot]

@jojochuang Thanks for the review! I'll fix it as soon as possible

rich7420 avatar Dec 06 '25 03:12 rich7420