SEPURI-SAI-KRISHNA commented on PR #18541: URL: https://github.com/apache/dolphinscheduler/pull/18541#issuecomment-5237031598
I had to revert the `appLink` reset — it regresses sub-workflow recovery, and CI caught it. `Unit-Test (dolphinscheduler-master | Java 11)` failed on `WorkflowInstanceRecoverStopTestCase#testRecoverStoppedWorkflow_with_subWorkflowTask_success` with the commit that cleared `appLink` ([job](https://github.com/apache/dolphinscheduler/actions/runs/31360840016/job/93369977204)). The reason is that `appLink` is overloaded — it is not only a remote application id: - For `AbstractRemoteTask` (Spark/Flink/EMR) it holds the remote application id, which is the case you pointed at. - For `SubWorkflowLogicTask` it holds the serialized `SubWorkflowLogicTaskRuntimeContext`, i.e. the **sub workflow instance id**: ```java // SubWorkflowLogicTask constructor this.subWorkflowLogicTaskRuntimeContext = JSONUtils.parseObject( taskExecutionContext.getAppIds(), SubWorkflowLogicTaskRuntimeContext.class); ``` ```java private SubWorkflowLogicTaskRuntimeContext initializeSubWorkflowInstance() { if (subWorkflowLogicTaskRuntimeContext == null) { return triggerNewSubWorkflow(); } switch (...getCommand().getCommandType()) { case RECOVER_TOLERANCE_FAULT_PROCESS: return recoverFromFaultTolerantTasks(); case RECOVER_SUSPENDED_PROCESS: return recoverFromSuspendTasks(); case START_FAILURE_TASK_PROCESS: return recoverFromFailedTasks(); default: return triggerNewSubWorkflow(); } } ``` Clearing `appLink` makes `subWorkflowLogicTaskRuntimeContext` null, so the recreated task short-circuits to `triggerNewSubWorkflow()` and spawns a **duplicate** sub workflow instead of recovering the existing one. Note this hits `START_FAILURE_TASK_PROCESS` too — the very command this PR is about — not just `RECOVER_SUSPENDED_PROCESS`. `RecoverSuspendWorkflowCommandHandler extends RecoverFailureTaskCommandHandler`, so both recovery commands go through `FailedRecoverTaskInstanceFactory` and both are affected. So this PR now keeps `appLink` as before, and I added a comment on the test assertion recording *why* it must be preserved, so the next person doesn't remove it for the same reason I did. `WorkflowInstanceRecoverStopTestCase` (2 tests) and `FailedRecoverTaskInstanceFactoryTest` (3 tests) both pass locally after the revert. ## On the underlying remote-task problem Your analysis of `AbstractRemoteTask#handle` is still correct — a recovered Spark/Flink task should not attach to the dead application of the previous attempt. But it can't be fixed by blanket-clearing `appLink` in the factory, because the same field carries the sub workflow runtime context. It needs something that distinguishes "resume this attempt" from "this is a new attempt", e.g. only clearing `appIds` for `AbstractRemoteTask` when the context is built, or giving the logic-task runtime context its own field instead of reusing `appLink`. That looks like its own issue rather than something to fold in here — happy to open one and work it if you agree. Would you prefer that, or should I keep trying to solve it inside this PR? -- 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]
