Refactor: convert RealtimeOffsetAutoResetHandler to interface with init(); fix nonLeaderCleanup by deeppatel710 · Pull Request #18127 · apache/pinot

deeppatel710 · 2026-04-08T06:41:56Z

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

…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 · 2026-04-08T06:45:00Z

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

Jackie-Jiang

Thanks for the contribution

codecov-commenter · 2026-04-08T21:49:08Z

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.12%. Comparing base (e7ea035) to head (8ed7dc5).
⚠️ Report is 80 commits behind head on master.

Files with missing lines	Patch %	Lines
...iodictask/RealtimeOffsetAutoResetKafkaHandler.java	0.00%	2 Missing ⚠️
...ler/validation/RealtimeOffsetAutoResetManager.java	77.77%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #18127      +/-   ##
============================================
- Coverage     63.96%   63.12%   -0.84%     
+ Complexity     1617     1616       -1     
============================================
  Files          3179     3213      +34     
  Lines        193741   195737    +1996     
  Branches      29926    30240     +314     
============================================
- Hits         123918   123551     -367     
- Misses        60031    62312    +2281     
- Partials       9792     9874      +82

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-11	`63.09% <63.63%> (-0.84%)`	⬇️
java-21	`63.10% <63.63%> (-0.84%)`	⬇️
temurin	`63.12% <63.63%> (-0.84%)`	⬇️
unittests	`63.11% <63.63%> (-0.84%)`	⬇️
unittests1	`55.34% <ø> (-0.51%)`	⬇️
unittests2	`34.77% <63.63%> (+0.43%)`	⬆️

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.

- 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 · 2026-04-09T04:59:04Z

@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 · 2026-04-09T23:44:39Z

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

xiangfu0

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

xiangfu0 · 2026-04-11T13:02:58Z

-      handler = (RealtimeOffsetAutoResetHandler) clazz.getConstructor(
-          PinotLLCRealtimeSegmentManager.class, PinotHelixResourceManager.class).newInstance(
-          _llcRealtimeSegmentManager, _pinotHelixResourceManager);
+      handler = (RealtimeOffsetAutoResetHandler) clazz.getConstructor().newInstance();


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.

@xiangfu0 Thanks for catching this! You're right — the loading change would break existing handlers that only have the 2-arg constructor.

Fix: getOrConstructHandler() now tries the new no-arg + init() pattern first, and falls back to the deprecated 2-arg constructor (with a WARN log prompting migration) if no no-arg constructor is found.
RealtimeOffsetAutoResetKafkaHandler also has the 2-arg constructor restored as @deprecated so existing subclasses remain source-compatible. Added a test that exercises this fallback path end-to-end.

@xiangfu0 how about now? can you review this again please? thank you

@xiangfu0 following up on this , can you please review and stamp this PR? thank you!

@deprecated

…etAutoResetHandler Handlers compiled against the previous constructor-injection SPI contract (PinotLLCRealtimeSegmentManager, PinotHelixResourceManager) would fail to instantiate after upgrade because the new loading code looks for a no-arg constructor. Fix: - getOrConstructHandler() now tries no-arg + init() first; if no no-arg constructor exists it falls back to the old 2-arg constructor with a WARN-level deprecation log, preserving behaviour for existing deployed plugins. - RealtimeOffsetAutoResetKafkaHandler re-adds the @deprecated 2-arg constructor (calling init() internally) so subclasses compiled against the old contract continue to compile and run. - Adds a LegacyRealtimeOffsetAutoResetHandler test fixture and a test that exercises the fallback path end-to-end. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

deeppatel710 mentioned this pull request Apr 8, 2026

Realtime ingestion offset auto reset #15782

Closed

Jackie-Jiang approved these changes Apr 8, 2026

View reviewed changes

Comment thread ...org/apache/pinot/controller/helix/core/periodictask/RealtimeOffsetAutoResetKafkaHandler.java Outdated

Jackie-Jiang added the refactor Code restructuring without changing behavior label Apr 8, 2026

xiangfu0 reviewed Apr 11, 2026

View reviewed changes

deeppatel710 requested a review from xiangfu0 April 19, 2026 06:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

deeppatel710 commented Apr 8, 2026 •

edited

Loading

Uh oh!

deeppatel710 commented Apr 8, 2026

Uh oh!

Jackie-Jiang left a comment

Uh oh!

Uh oh!

codecov-commenter commented Apr 8, 2026 •

edited

Loading

Uh oh!

deeppatel710 commented Apr 9, 2026

Uh oh!

deeppatel710 commented Apr 9, 2026

Uh oh!

xiangfu0 left a comment

Uh oh!

xiangfu0 Apr 11, 2026

Uh oh!

deeppatel710 Apr 12, 2026

Uh oh!

deeppatel710 Apr 14, 2026

Uh oh!

deeppatel710 Apr 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

deeppatel710 commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Labels

Uh oh!

deeppatel710 commented Apr 8, 2026

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

deeppatel710 commented Apr 9, 2026

Uh oh!

deeppatel710 commented Apr 9, 2026

Uh oh!

xiangfu0 left a comment

Choose a reason for hiding this comment

Uh oh!

xiangfu0 Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

deeppatel710 Apr 12, 2026

Choose a reason for hiding this comment

Uh oh!

deeppatel710 Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

deeppatel710 Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

deeppatel710 commented Apr 8, 2026 •

edited

Loading

codecov-commenter commented Apr 8, 2026 •

edited

Loading