[Improvement-16612][Master] For logical tasks on the Master, there should be support for dry run #16616
Conversation
shouwangyw
requested review from
caishunfeng,
SbloodyS and
ruanwenjun
as code owners
SbloodyS
added
improvement
Okay, no problem, I will refer to the previous test cases. |
|
It's needed to add integration test case to test the whole runtime case rather than ut. |
Okay, but can you tell me how to run these integration test cases? They all throw errors when I run them on my local IDEA. The error info： |
I found the real reason for the integration test error. Some fields in the YAML file need to be renamed, for example: processDefinitionCode needs to be replaced with workflowDefinitionCode, and processDefinitionVersion needs to be replaced with workflowDefinitionVersion, etc. @ruanwenjun And there are some errors in the assertions as well. |
It's better to submit another PR to fix these IT. |
|
This is caused by the IT case has been skipped in ci, I will fixed this. |
|
This problem has been fixed by #16637, please rebase from upstream and add new case |
|
|
||
| @Test | ||
| @DisplayName("Test start a workflow with one sub workflow task(A) dry run success") | ||
| public void testStartWorkflow_with_subWorkflowTask_dryRunSuccess() { |
There was a problem hiding this comment.
Why the parent workflow using dry run but still generate sub workflow instance? If use dry run then the SubWorkflow task will not generate sub workflow instance.
There was a problem hiding this comment.
Why the parent workflow using dry run but still generate sub workflow instance? If use dry run then the SubWorkflow task will not generate sub workflow instance.
Child workflow inherits the dry run configuration of the parent workflow.
This is indeed the effect, and the sub-workflow will also dry run.
There was a problem hiding this comment.
This might make confusion, since physical task dry run will not go into the task plugin logic, you can find the implementation in WorkerTaskExecutor. It's better to keep this consistent.
You should do this change in MasterTaskExecutor, otherwise the other logic task still doesn't support dry run.
There was a problem hiding this comment.
This might make confusion, since physical task dry run will not go into the task plugin logic, you can find the implementation in
WorkerTaskExecutor. It's better to keep this consistent.You should do this change in
MasterTaskExecutor, otherwise the other logic task still doesn't support dry run.
OK, I'll check it out.
There was a problem hiding this comment.
This might make confusion, since physical task dry run will not go into the task plugin logic, you can find the implementation in
WorkerTaskExecutor. It's better to keep this consistent.You should do this change in
MasterTaskExecutor, otherwise the other logic task still doesn't support dry run.
Check whether it is dry run in AsyncMasterTaskDelayQueueLooper, it may also be necessary!
| AsyncTaskExecuteFunction.AsyncTaskExecutionStatus asyncTaskExecutionStatus; | ||
| if (taskExecutionContext.getDryRun() == DRY_RUN_FLAG_YES) { | ||
| log.info( | ||
| "The current execute mode is dry run check again, will stop the logic task and set the taskInstance status to success"); | ||
| asyncTaskExecutionStatus = AsyncTaskExecuteFunction.AsyncTaskExecutionStatus.SUCCESS; | ||
| } else { | ||
| asyncTaskExecutionStatus = asyncTaskExecuteFunction.getAsyncTaskExecutionStatus(); | ||
| } |
There was a problem hiding this comment.
We can revert this change, since the Async task will not put into asyncMasterTaskDelayQueue
There was a problem hiding this comment.
We can revert this change, since the Async task will not put into asyncMasterTaskDelayQueue
This is for code robustness considerations. I'm not sure if there are other places where events can be sent.
| if (DRY_RUN_FLAG_YES == taskExecutionContext.getDryRun()) { | ||
| taskExecutionContext.setCurrentExecutionStatus(TaskExecutionStatus.SUCCESS); | ||
| taskExecutionContext.setEndTime(System.currentTimeMillis()); | ||
| MasterTaskExecutorHolder.removeMasterTaskExecutor(taskExecutionContext.getTaskInstanceId()); | ||
| logicTaskInstanceExecutionEventSenderManager.successEventSender().sendMessage(taskExecutionContext); | ||
| log.info( | ||
| "The current execute mode is dry run, will stop the logic task and set the taskInstance status to success"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
It's better to move this after beforeExecute I am not sure if the statemachine will throw exception, since the task lifecycle might miss running event, so the task state will be dispatched, and dispatched task cannot be converted to success.
There was a problem hiding this comment.
It's better to move this after
beforeExecuteI am not sure if the statemachine will throw exception, since the task lifecycle might missrunningevent, so the task state will be dispatched, and dispatched task cannot be converted to success.
I find the implementation in WorkerTaskExecutor,this is before beforeExecute.
There was a problem hiding this comment.
Test Result：
This is not the effect we want.
There was a problem hiding this comment.
I have change, test is ok
|
|
Purpose of the pull request
This pull request improve support for dry run for logical tasks on the Master
fix #16612
Brief change log
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(or)
Pull Request Notice
Pull Request Notice
If your pull request contain incompatible change, you should also add it to
docs/docs/en/guide/upgrede/incompatible.md