njnu-seafish commented on PR #18549: URL: https://github.com/apache/dolphinscheduler/pull/18549#issuecomment-5477232207
> #### `TaskExecutionContext.getVarPool()` is not populated with the predecessor-scoped VarPool > The latest change uses: > > ``` > taskExecutionContext.getVarPool() > ``` > > However, `TaskExecutionContextFactory.createTaskExecutionContext()` only writes the result of `generateTaskInstanceVarPool()` to: > > ``` > taskInstance.setVarPool(VarPoolUtils.serializeVarPool(varPools)); > ``` > > `TaskExecutionContextBuilder.buildTaskInstanceRelatedInfo()` does not copy `taskInstance.varPool` into `TaskExecutionContext`, and `TaskExecutionContext.varPool` has no default value. Therefore, for a newly initialized sub-workflow logic task, `taskExecutionContext.getVarPool()` is normally null. > > As a result, the current one-line change still drops runtime OUT parameters from upstream tasks. A manual test may appear to pass when the same parameter is also present in global parameters or the original workflow start parameters, but it does not verify propagation from the predecessor task's runtime output. > > Please explicitly propagate the predecessor-scoped VarPool into the task execution context, or read the scoped VarPool from the current task instance. Also add automated regression tests covering: > > 1. An OUT parameter produced only at runtime by an upstream task is passed to the sub-workflow. > 2. An OUT parameter from an unrelated sibling branch is not passed to the sub-workflow. > 3. Conflicting global/start/upstream parameters retain the intended precedence. Thanks @SbloodyS. I'd like to clarify the scope here, because I believe this concern is out of scope for this PR. .varPool(taskExecutionContext.getVarPool()) is pre-existing code on dev. In the diff of this PR it appears as an unchanged context line; this PR only adds the two adjacent lines .needAlert(...) and .taskAlertInfo(...): .varPool(taskExecutionContext.getVarPool()) // unchanged (pre-existing on dev) + .needAlert(taskExecutionContext.isNeedAlert()) // added by this PR + .taskAlertInfo(taskExecutionContext.getTaskAlertInfo()) // added by this PR This PR does not modify TaskExecutionContextFactory or TaskExecutionContextBuilder. The alert path is decoupled from varPool. The SQL task-result alert travels through needAlert / taskAlertInfo, an independent channel that does not read taskExecutionContext.getVarPool(). SqlTask#prepareTaskResultAlert() only sets the alert info; it never reads varPool. So this PR neither affects nor depends on varPool propagation. The varPool-propagation issue on sub-workflow logical tasks looks like a real pre-existing bug on dev, and I agree it's worth fixing — but it touches the shared TaskExecutionContext build path that all task types go through, so it belongs in a separate issue/PR with its own regression tests, rather than being mixed into an alert-focused PR (close #17854). Could you double-check whether this comment was intended for another PR that actually touches the varPool/sub-workflow path? -- 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]
