Skip to content

Conversation

@suvodeep-pyne
Copy link
Contributor

Summary

This PR refactors the reload job status logic in the controller by extracting it from PinotTableReloadService into a new dedicated PinotTableReloadStatusReporter class. This improves separation of concerns and makes the codebase more maintainable.

Key Changes

  1. Created new PinotTableReloadStatusReporter class

    • Extracted getReloadJobStatus() method from PinotTableReloadService
    • Extracted getServerToSegments() helper method
    • Added helper methods for computing time metrics: computeTimeElapsedInMinutes(), computeEstimatedRemainingTimeInMinutes(), computeTotalSegments()
    • Registered as a Singleton in BaseControllerStarter
  2. Updated PinotTableReloadResource

    • Injected PinotTableReloadStatusReporter alongside existing PinotTableReloadService
    • Delegated reload job status queries to the new reporter class
    • Renamed field _pinotTableReloadService to _service for brevity
    • Renamed method getTableReloadMetadata() to needReload() for clarity
  3. Refactored ServerReloadControllerJobStatusResponse

    • Converted all setters to fluent style (return this) to enable method chaining
    • This allows for cleaner initialization in the new reporter class
  4. Simplified PinotTableReloadService

    • Removed reload status reporting logic (moved to reporter)
    • Kept core reload functionality focused on actual reload operations
    • Maintained needReload() method (renamed from getTableReloadMetadata())
  5. Updated test class

    • Renamed PinotTableReloadServiceTest to PinotTableReloadStatusReporterTest
    • Updated to test the new reporter class

Benefits

  • Better separation of concerns: Service now focuses on reload operations, reporter on status reporting
  • Improved code organization: Status reporting logic is isolated and easier to maintain
  • Fluent API: Method chaining in response objects makes code more readable
  • No functional changes: Only refactoring, behavior remains the same

Test Plan

  • Existing unit tests updated and passing
  • No API changes (endpoint paths and responses remain the same)

@suvodeep-pyne suvodeep-pyne changed the title [controller] Refactor reload job status logic into a dedicated PinotTableReloadStatusReporter class [controller] Refactor. Moved reload job status logic into a dedicated PinotTableReloadStatusReporter class Oct 17, 2025
@suvodeep-pyne
Copy link
Contributor Author

suvodeep-pyne commented Oct 17, 2025

@sonam
refactor PR. moved code. +cosmetic refactors. no logic change.

Copy link
Contributor

@somandal somandal left a comment

Choose a reason for hiding this comment

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

lgtm!

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2025

Codecov Report

❌ Patch coverage is 19.19192% with 80 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.50%. Comparing base (661b1d0) to head (3135b25).

Files with missing lines Patch % Lines
...oller/services/PinotTableReloadStatusReporter.java 21.68% 64 Missing and 1 partial ⚠️
...urces/ServerReloadControllerJobStatusResponse.java 0.00% 8 Missing ⚠️
...roller/api/resources/PinotTableReloadResource.java 0.00% 7 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #17036      +/-   ##
============================================
+ Coverage     63.44%   63.50%   +0.05%     
  Complexity     1419     1419              
============================================
  Files          3083     3084       +1     
  Lines        182133   182142       +9     
  Branches      27952    27952              
============================================
+ Hits         115558   115662     +104     
+ Misses        57663    57572      -91     
+ Partials       8912     8908       -4     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-11 63.47% <19.19%> (+7.20%) ⬆️
java-21 63.44% <19.19%> (+0.02%) ⬆️
temurin 63.50% <19.19%> (+0.05%) ⬆️
unittests 63.49% <19.19%> (+0.05%) ⬆️
unittests1 56.31% <ø> (+0.02%) ⬆️
unittests2 33.65% <19.19%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@somandal somandal merged commit 9c2948b into apache:master Oct 17, 2025
18 checks passed
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