Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix-16768] Add scheduleTime parameter to sub-workflow instance. #16776

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

reele
Copy link
Contributor

@reele reele commented Nov 6, 2024

Purpose of the pull request

Fix #16768

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

SubWorkflow_log

Pull Request Notice

Just fix bug.

@SbloodyS SbloodyS changed the title Fix #16768, add scheduleTime parameter to sub-workflow instance. [Fix-16768] Add scheduleTime parameter to sub-workflow instance. Nov 6, 2024
Copy link

sonarcloud bot commented Nov 6, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
14.2% Duplication on New Code (required ≤ 10%)

See analysis details on SonarCloud

Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ruanwenjun ruanwenjun added bug Something isn't working 3.3.0 labels Nov 6, 2024
@ruanwenjun ruanwenjun added this to the 3.3.0 milestone Nov 6, 2024
@reele
Copy link
Contributor Author

reele commented Nov 6, 2024

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?

I'd like to abstract out a execution init parameter entity(like command object) and reference it, instead of add a value field

@ruanwenjun
Copy link
Member

@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.

@reele
Copy link
Contributor Author

reele commented Nov 7, 2024

@ruanwenjun i can have a try

@reele
Copy link
Contributor Author

reele commented Nov 11, 2024

@ruanwenjun
I spent some time reading through the main code, and I think directly write scheduleTime with prop system.schedule.time into ScheduleWorkflowCommandParam.commandParams in WorkflowScheduleTrigger.constructTriggerCommand is the better than add a new entity.

It's always propagated to sub-workflows. At runtime, it will be merged into globalParams, and easily to retrieve scheduleTime with minimal code changes.

then refactor some date operations like CuringParamsServiceImpl.preBuildBusinessParams or BusinessTimeUtils.getBusinessTime to use system.schedule.time to calculate dates.

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: [Bug] [TaskExecutionRunnable] Sub workflow task always repeat run in master-server failover #16767

  2. Some task or parameter operations do not verify whether the workflow is a sub-workflow. As a result, resulting in many workflowInstance.getScheduleTime() calls may needing assessment. I modified several code sections to use commandParams instead:dev...reele:dolphinscheduler:fix-16768-2

  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.

  4. The last issue is bigger! dependent task use workflowInstance.scheduleTime == null to determine if the workflow is manual run, there can not replace with builtin parameter, that will cause db performance issue, so scheduleTime need always set a value in sub-workflow(from parent instance)
    there need a value to get the initial command type, or change to another way to implement the dependent logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.3.0 backend bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] [SubWorkflowLogicTask] missing scheduleTime param during schedule or run complement in sub-workflow
2 participants