re20052 commented on PR #66173: URL: https://github.com/apache/doris/pull/66173#issuecomment-5472997224
> Thanks for the fix. I found two remaining concerns: > > 1. **[P1] CREATE-log replay can become terminal before later state records are applied.** `gsonPostProcess()` is also invoked by `RoutineLoadJob.read()` for `OP_CREATE_ROUTINE_LOAD_JOB`. If parsing/validation fails, the CREATE record is materialized as `CANCELLED`; a subsequent `OP_CHANGE_ROUTINE_LOAD_JOB` calls `updateState()`, but `checkStateTransform()` rejects every transition from a final state. Consequently, the authoritative later state—and in particular the durable cancellation reason added by this PR—can be dropped during journal replay. `replayCreateRoutineLoadJob()` also registers this synthetic final job as a transaction callback. Please distinguish image recovery from CREATE-log deserialization, or otherwise make later replay operations authoritative, and add a byte-level CREATE → CHANGE → REMOVE replay test. > 2. **[P2] The generated reason can report the wrong failure time.** When a pre-existing final job already has an `endTimestamp` but lacks a persisted `cancelReason`, the message uses that historical stop/cancel timestamp as the time of the current FE restart deserialization failure. Please capture `failureTimestamp = System.currentTimeMillis()` for the message, while assigning it to `endTimestamp` only when the latter is unset. > > The current checks are green, but the added tests do not exercise the byte-level journal replay sequence above. Thanks for the review. [P2] Fixed in 74c4653 — the message now uses a failureTimestamp = System.currentTimeMillis() captured at failure time, while endTimestamp is assigned only when unset. [P1] I'd prefer not to change this here. Terminating a job as CANCELLED when its CREATE statement can no longer be parsed is pre-existing behavior; this PR only persists the cancel reason and doesn't touch the state machine or the replay path. More importantly, making a later replay override the terminal state would break a core invariant — CANCELLED is irreversible by design (checkStateTransform rejects any transition out of it), so a cancelled job must never be revived on replay. The behavior is also reasonable: an unparseable CREATE means the job is genuinely unusable, and a "deserialize failed" reason reflects the real failure better than a stale one. I'd treat replay fidelity here as a separate, pre-existing concern. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
