3424672656 commented on PR #11157:
URL: https://github.com/apache/rocketmq/pull/11157#issuecomment-5748894438

   > > _This was generated by AI during triage._
   > 
   > I took another look at the latest head (`b3b7490`). This PR is addressing 
a real correctness/performance issue in `TimelineRollService`: the old 
fixed-sleep loop scanned a window like `[now + rollRange, now + rollRange + 
timerMaxDelaySec]`, so adjacent roll rounds overlapped heavily and could roll 
the same far-future timer records back to `TIMER_TOPIC` repeatedly. Moving to a 
checkpoint-driven scan (`[checkpoint, checkpoint + rollRange)`) with a 
dedicated `timeline_roll_checkpoint` is the right direction.
   > 
   > The latest version also fixes the earlier crash-skip concern by moving 
checkpoint persistence into `TimerMessageReputService`, after the queued roll 
batch has been processed, instead of writing it immediately after scan.
   > 
   > One remaining correctness concern before merge: `TimerMessageReputService` 
writes the checkpoint after all task countdowns complete, but individual tasks 
do not report whether `putMsgWithRetry()` actually succeeded. 
`putMsgWithRetry()` currently returns `void` and logs/returns after retries are 
exhausted. If one or more records fail to re-put to the commitlog, the service 
can still advance `TIMELINE_ROLL_CHECK_POINT` past that window, which removes 
the normal early-roll retry opportunity for those failed records.
   > 
   > Suggested fix: make each task return success/failure and only write the 
roll checkpoint when the whole batch succeeds, or avoid advancing the 
checkpoint past failed records.
   > 
   > One compatibility note: this PR removes `timerRocksDBRollIntervalHours`, 
and `timerRocksDBRollRangeHours` now effectively controls both the scan window 
width and roll cadence. Please document this in config docs/release notes, 
since deployments with a custom roll interval may see changed behavior.
   > 
   > CI is green and the overall checkpoint-based design looks sound once the 
failed-reput checkpoint edge is handled.
   
   fix 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to