[fix](routine load) fix NPE when replaying change routine load job fo…#65555
[fix](routine load) fix NPE when replaying change routine load job fo…#65555enocknkhoma wants to merge 1 commit into
Conversation
…r non-existent job in cloud mode Add null check for routine load job before setting cloud cluster.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-H: Total hot run time: 29743 ms |
TPC-DS: Total hot run time: 179522 ms |
ClickBench: Total hot run time: 24.98 s |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one replay-equivalence issue that should be fixed before merge.
Part 1.3 checkpoint conclusions:
- Goal/test: the patch prevents the cloud replay NPE for an orphaned routine-load change record, but it does not preserve all state cleanup from the original final-state transition. No automated test for the cloud missing-job replay/callback case is added; I did not run builds or tests because this review runner is review-only.
- Scope: the code change is small and localized to
CloudRoutineLoadManager.replayChangeRoutineLoadJob(), but the affected behavior crosses image load, edit-log replay, old-job cleanup, and transaction callbacks. - Concurrency/lifecycle/persistence: a remove-before-change replay ordering is valid when final-state update and old-job cleanup interleave; the accepted inline comment covers the missing callback cleanup in that lifecycle.
- Config/compatibility/FE-BE/protocol: no new config, persisted format, or FE-BE protocol fields are introduced.
- Parallel paths/observability/performance: non-cloud replay already avoids crashing after the null dereference is caught; cloud replay now needs equivalent cleanup semantics. The warning is useful and there is no hot-path performance concern.
- User focus: no additional user-provided focus points were present.
Two subagent rounds completed. After accepting the callback-cleanup issue, both convergence subagents returned NO_NEW_VALUABLE_FINDINGS, and there are no remaining unresolved candidates.
| if (job == null) { | ||
| LOG.warn("routine load job {} does not exist when replaying change routine load job, ignore it", | ||
| operation.getId()); | ||
| return; |
There was a problem hiding this comment.
This makes the orphaned change record replayable, but it still leaves the transaction callback state different from the original master. A valid ordering for this failure is: the job is loaded from image as non-final, so readFields() registers its txn callback; cleanup replays OP_REMOVE_ROUTINE_LOAD_JOB first and removes the job from idToRoutineLoadJob but does not remove that callback; then this branch sees the later OP_CHANGE_ROUTINE_LOAD_JOB and returns before updateState(..., isReplay=true) can perform the final-state removeCallback(id). After replay continues, TransactionState.replaySetTransactionStatus() and the cloud commit path look callbacks up directly by id, so that stale RoutineLoadJob can still receive txn callbacks even though the manager no longer contains the job. Please clear the callback in this missing-job path, or preferably in replayRemoveOldRoutineLoad(), before ignoring the orphaned change.
|
Thanks for making the replay path tolerant of a missing routine load job. This prevents FE startup from being blocked by an orphan However, this appears to address the replay symptom without explaining how the orphan journal entry is generated. Could you please clarify the root cause and the exact ordering that allows a change operation to remain after the job has been removed? For example, is there a race between transitioning a job to a final state and If Doris can still generate this ordering, should we also fix the journal ordering or locking to prevent new orphan entries, instead of relying only on replay-time tolerance? A focused unit test covering the missing-job replay case would also be helpful. |
4 participants
What problem does this PR solve?
Issue Number: No related issue
Related PR: #40728, #53768
Problem Summary:
In compute-storage decoupled (cloud) mode, if the edit log contains an OP_CHANGE_ROUTINE_LOAD_JOB (Operation Type 201) entry whose routine load job no longer exists in memory (the job was cleaned up while a later change op for it remains in the journal), FE throws a NullPointerException during journal replay and cannot start. All FEs replaying the same journal crash
identically, so the entire FE cluster becomes unrecoverable.
force_skip_journal_idcannot bypass it, as it only covers journals that fail to deserialize, not replay exceptions.Observed in production on 3.1.0 (replayer thread):
Reproduced identically on 3.1.1 and 3.1.4 (on 3.1.4 also via the state transfer path during master election: Env.transferToMaster -> Env.replayJournal, same NPE at CloudRoutineLoadManager.java:70). The unguarded call is still present in 4.1.2 and master.
#40728 and #53768 fixed the illegal-state and null-field cases in this same replay path; this PR covers the remaining null-job case with the same defensive pattern (as used in CloudSystemInfoService.replayModifyBackend).
Note: the base RoutineLoadManager.replayChangeRoutineLoadJob has the same unguarded getJob() dereference on the non-cloud path; this PR fixes the cloud override where the crash was observed, the base class can be guarded in a follow-up.
Fix: null-check the job and skip the edit log entry with a warning, consistent with how a change op for an already-removed job should be treated.
Release note
Fix FE start failure (NPE) in cloud mode when replaying OP_CHANGE_ROUTINE_LOAD_JOB for a routine load job that no longer exists.
Check List (For Author)
Test
Replayed a production edit log containing an orphaned OP_CHANGE_ROUTINE_LOAD_JOB entry in a 3-FE cloud-mode sandbox (FDB + Meta Service). Without this fix, all FEs crash-loop at the same log id (verified on 3.1.0/3.1.1/3.1.4). With the fix, the FE logs a warning, replay completes, master election succeeds, and routine load jobs resume normally.
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)