You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by "reele (via GitHub)" <gi...@apache.org> on 2024/11/06 04:12:28 UTC
[PR] Fix #16768, add `scheduleTime` parameter to sub-workflow instance. [dolphinscheduler]
reele opened a new pull request, #16776:
URL: https://github.com/apache/dolphinscheduler/pull/16776
## Purpose of the pull request
add `scheduleTime` parameter to sub-workflow instance.
## Brief change log
refactored `SubWorkflowManualTrigger` to `SubWorkflowTrigger` (only for sub-workflow, but it is very similar with `WorkflowManualTrigger`)
added `SubWorkflowTriggerRequest`,`SubWorkflowTriggerResponse`
## Verify this pull request
This pull request is code cleanup without any test coverage.
I made a test which run a dependent task in sub-workflow to get original schedule-time parameter:
[DependentTask_log](https://github.com/user-attachments/files/17641488/1730865848091.log)
[SubWorkflow_log](https://github.com/user-attachments/files/17641540/1730866071948.log)
## Pull Request Notice
Just fix bug.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16768] Add `scheduleTime` parameter to sub-workflow instance. [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on PR #16776:
URL: https://github.com/apache/dolphinscheduler/pull/16776#issuecomment-2461252239
@reele Yes, we should avoid add a field value of schedule time in task, since this is only related to runtime info, it's better we can get the scheduler time from runtime info. We can use a built-in parameter to represent the schedule time.
Could you please provide a design first, the design is important to help to make the work clearer.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16768] Add `scheduleTime` parameter to sub-workflow instance. [dolphinscheduler]
Posted by "reele (via GitHub)" <gi...@apache.org>.
reele commented on PR #16776:
URL: https://github.com/apache/dolphinscheduler/pull/16776#issuecomment-2468487492
@ruanwenjun
I spent some time reading through the main code, and I believe we can directly write `scheduleTime` into `ScheduleWorkflowCommandParam` in `WorkflowScheduleTrigger.constructTriggerCommand`. It will be passed to `commandParams` and always propagated to sub-workflows. At runtime, it will be merged into `globalParams`, it‘s easy to retrieve `scheduleTime` with minimal changes.
But there are still a few issues to resolve:
1. The master's failover process always takes over all workflows, including sub-workflows. So, when a sub-workflow is taken over, it can also retrieve the schedule time from `globalParams`. But there's an issue with failing over sub-workflow tasks; it triggers a new sub-workflow instance to run alongside the original master-taken-over sub-workflow. I prefer to directly take over the sub-workflow task rather than triggering a new sub-workflow, as the sub-workflow may contain many tasks. The issue is: https://github.com/apache/dolphinscheduler/issues/16767
2. Some task or parameter operations do not verify whether the workflow is a sub-workflow. As a result, numerous instances of workflowInstance.getScheduleTime() need to be assessed to decide whether they are within a sub-workflow or should be altered to utilize commandParams/globalParams operations. When I coded according to the latter approach, I modified 17 sections of the code. So do we really need to proceed in this manner for store schedule time for sub-workflow?
3. Currently, parameter runtime values are obtained in real-time, while `globalParams` stores the definition values. This causes the parameter view on the UI page to display the definition value rather than the runtime value. and also I think storing the runtime value may help with instance failover.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16768] Add `scheduleTime` parameter to sub-workflow instance. [dolphinscheduler]
Posted by "reele (via GitHub)" <gi...@apache.org>.
reele commented on PR #16776:
URL: https://github.com/apache/dolphinscheduler/pull/16776#issuecomment-2459710122
> Add schedule time to sub workflow is very strange, the parent schedule time doesn't relate to sub workflow, like we don't add a schedule time to task. If we add schedule time to sub workflow, it would be better we also add schedule time in task. This may be a problem with our use of schedule time, we should be able to get the schedule time during workflow runtime, no matter it is a task or sub workflow.
Allow task nodes to access initial runtime parameters, either by storing the parameters directly or by recording where to retrieve them.
However, the multi-layer `sub-workflow` scheduling relationship complicates the parameter retrieval process, especially during the `master`'s `failover` process (currently, it doesn't restore tasks through the scheduling relationship).
This is essentially the same issue as recording `command-param`.
if we aim for a purely paradigmatic approach, we should treat `command` as an independent entity and only store the `command_id` in the top-level `workflow_instance`. Then, tasks in sub-workflows would need to recursively traverse both `relation_workflow_instance` and `workflow_task_relation` to locate the top-level `workflow_instance` and subsequently the `command`
the key lies in balancing the distribution of information to optimize efficiency, stability, and operability.
so i still want to know what your thoughts are for the next step.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16768] Add `scheduleTime` parameter to sub-workflow instance. [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #16776:
URL: https://github.com/apache/dolphinscheduler/pull/16776#issuecomment-2458805737
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=16776) **Quality Gate failed**
Failed conditions
 [14.2% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=16776&metric=new_duplicated_lines_density&view=list) (required ≤ 10%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=16776)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16768] Add `scheduleTime` parameter to sub-workflow instance. [dolphinscheduler]
Posted by "reele (via GitHub)" <gi...@apache.org>.
reele closed pull request #16776: [Fix-16768] Add `scheduleTime` parameter to sub-workflow instance.
URL: https://github.com/apache/dolphinscheduler/pull/16776
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16768] Add `scheduleTime` parameter to sub-workflow instance. [dolphinscheduler]
Posted by "reele (via GitHub)" <gi...@apache.org>.
reele commented on PR #16776:
URL: https://github.com/apache/dolphinscheduler/pull/16776#issuecomment-2460086234
> Add schedule time to sub workflow is very strange, the parent schedule time doesn't relate to sub workflow, like we don't add a schedule time to task. If we add schedule time to sub workflow, it would be better we also add schedule time in task. This may be a problem with our use of schedule time, we should be able to get the schedule time during workflow runtime, no matter it is a task or sub workflow.
I'll try to set schedule time on task...
But, currently schedule time is used for generating global parameters in workflow and calculating dates for dependent task.
The execution of a task is entirely dependent on the workflow instance, so it can always got the schedule time.
Therefore, What confuses me is `What the effect would be if we add schedule time to the task`?
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16768] Add `scheduleTime` parameter to sub-workflow instance. [dolphinscheduler]
Posted by "reele (via GitHub)" <gi...@apache.org>.
reele commented on PR #16776:
URL: https://github.com/apache/dolphinscheduler/pull/16776#issuecomment-2461323588
@ruanwenjun i can have a try
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Fix-16768] Add `scheduleTime` parameter to sub-workflow instance. [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #16776:
URL: https://github.com/apache/dolphinscheduler/pull/16776#issuecomment-2458807276
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=16776) **Quality Gate failed**
Failed conditions
 [14.2% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=16776&metric=new_duplicated_lines_density&view=list) (required ≤ 10%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=16776)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org