Skip to content

Refactor: convert RealtimeOffsetAutoResetHandler to interface with init(); fix nonLeaderCleanup#18127

Open
deeppatel710 wants to merge 2 commits intoapache:masterfrom
deeppatel710:jackie-jiang-review-feedback
Open

Refactor: convert RealtimeOffsetAutoResetHandler to interface with init(); fix nonLeaderCleanup#18127
deeppatel710 wants to merge 2 commits intoapache:masterfrom
deeppatel710:jackie-jiang-review-feedback

Conversation

@deeppatel710
Copy link
Copy Markdown
Contributor

@deeppatel710 deeppatel710 commented Apr 8, 2026 •
edited
Loading

Addresses review feedback from #15782 (Jackie-Jiang) that was not carried over when the feature was split and merged as #16492, #16692, and #16724:

  • Removed redundant public abstract modifiers from RealtimeOffsetAutoResetHandler interface methods; updated init() javadoc to reflect it is called after
    instantiation, not in a constructor
  • Removed constructor injection from RealtimeOffsetAutoResetKafkaHandler; initialization now happens via init() called by the manager after reflective
    instantiation
  • Fixed ensureBackfillJobsRunning signature: List<String> → Collection<String> to match the interface contract
  • Updated RealtimeOffsetAutoResetManager.getOrConstructHandler() to use no-arg constructor + init() instead of constructor injection via reflection
  • Fixed bug in nonLeaderCleanup(): was not clearing _tableBackfillTopics, leaving stale state when the controller re-acquires leadership

Changes

File Change
RealtimeOffsetAutoResetHandler.java Abstract class → interface; added init()
RealtimeOffsetAutoResetKafkaHandler.java Implements interface; removed constructor; fixed Collection<String> signature
RealtimeOffsetAutoResetManager.java Uses no-arg constructor + init(); fixed nonLeaderCleanup()

Labels

refactor, release-notes

@claude
Refactor: convert RealtimeOffsetAutoResetHandler to interface with in…
1c55cb4 
…it(); fix nonLeaderCleanup

- Remove redundant public abstract modifiers from RealtimeOffsetAutoResetHandler interface
- Update init() javadoc: "called once in constructor" -> "called once after instantiation"
- Remove constructor injection from RealtimeOffsetAutoResetKafkaHandler; init() is now the
  sole initialization path called by the manager after no-arg reflective instantiation
- Fix ensureBackfillJobsRunning signature: List<String> -> Collection<String> to match interface
- Update RealtimeOffsetAutoResetManager.getOrConstructHandler() to use no-arg constructor + init()
- Fix bug in nonLeaderCleanup(): also clear _tableBackfillTopics to avoid stale state on re-election
- Fix misleading error message: "Custom analyzer" -> "Custom handler"

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@deeppatel710 deeppatel710 mentioned this pull request Apr 8, 2026
Closed
@deeppatel710
Copy link
Copy Markdown
Contributor Author

deeppatel710 commented Apr 8, 2026

@Jackie-Jiang this addresses your review feedback from #15782. Would appreciate a review!

Jackie-Jiang
Jackie-Jiang approved these changes Apr 8, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution

@Jackie-Jiang Jackie-Jiang added the refactor Code restructuring without changing behavior label Apr 8, 2026
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 8, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.89%. Comparing base (e7ea035) to head (1c55cb4).
⚠️ Report is 5 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (e7ea035) and HEAD (1c55cb4). Click for more details.

HEAD has 6 uploads less than BASE
Flag BASE (e7ea035) HEAD (1c55cb4)
unittests 4 2
temurin 9 8
java-11 5 4
unittests2 2 0
Additional details and impacted files 
@@             Coverage Diff              @@
##             master   #18127      +/-   ##
============================================
- Coverage     63.96%   55.89%   -8.07%     
+ Complexity     1617      829     -788     
============================================
  Files          3179     2484     -695     
  Lines        193741   142663   -51078     
  Branches      29926    22975    -6951     
============================================
- Hits         123918    79746   -44172     
+ Misses        60031    56102    -3929     
+ Partials       9792     6815    -2977
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-11 55.84% <ø> (-8.09%) ⬇️
java-21 55.87% <ø> (-8.07%) ⬇️
temurin 55.89% <ø> (-8.07%) ⬇️
unittests 55.89% <ø> (-8.07%) ⬇️
unittests1 55.89% <ø> (+0.04%) ⬆️
unittests2 ?

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.

@claude
Address review feedback: remove redundant abstract method and fix tests
7cce6bf 
- Remove redundant `ensureBackfillJobsRunning` abstract override from
  RealtimeOffsetAutoResetKafkaHandler; the interface already declares it
- Fix TestRealtimeOffsetAutoResetHandler to use no-arg constructor so
  reflection-based instantiation in getOrConstructHandler() works correctly
- Strengthen testNonLeaderCleanup to assert handler is removed from
  internal map after nonLeaderCleanup is called

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@deeppatel710
Copy link
Copy Markdown
Contributor Author

deeppatel710 commented Apr 9, 2026

@Jackie-Jiang Thanks for the approval! I've addressed your feedback in the latest commit (removed the redundant abstract override and fixed the test to use a no-arg constructor). CI workflow needs a maintainer to approve it — could you approve the workflow run and merge when ready? Thanks!

@deeppatel710
Copy link
Copy Markdown
Contributor Author

deeppatel710 commented Apr 9, 2026

@yashmayya can you run the CI here as well? Thanks!!

xiangfu0
xiangfu0 reviewed Apr 11, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@xiangfu0 xiangfu0 left a comment

Choose a reason for hiding this comment

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

Found one high-signal backward-compatibility issue; see inline comment.

...ler/src/main/java/org/apache/pinot/controller/validation/RealtimeOffsetAutoResetManager.java
handler = (RealtimeOffsetAutoResetHandler) clazz.getConstructor(
PinotLLCRealtimeSegmentManager.class, PinotHelixResourceManager.class).newInstance(
_llcRealtimeSegmentManager, _pinotHelixResourceManager);
handler = (RealtimeOffsetAutoResetHandler) clazz.getConstructor().newInstance();
Copy link
Copy Markdown
Contributor

@xiangfu0 xiangfu0 Apr 11, 2026

Choose a reason for hiding this comment

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

This changes the auto-reset handler SPI from constructor injection to no-arg construction plus init(). Any existing custom handler compiled against the previous constructor-based contract will now fail to instantiate after upgrade, which is a backward-compatibility break for deployed plugins. Please keep a compatibility path for the legacy constructor (or add a shim/deprecation layer) before switching existing tables to the new loading flow.

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

Reviewers

@xiangfu0 xiangfu0 xiangfu0 left review comments

@Jackie-Jiang Jackie-Jiang Jackie-Jiang approved these changes

Assignees

No one assigned

Labels

refactor Code restructuring without changing behavior

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@deeppatel710 @codecov-commenter @xiangfu0 @Jackie-Jiang