1816: getNextTask will get wrong task if DO_WHILE task is embed in FORK or DECISION #1823
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1823 +/- ##
=============================================
- Coverage 65.07% 23.74% -41.34%
- Complexity 3897 4054 +157
=============================================
Files 301 372 +71
Lines 18483 79077 +60594
Branches 1684 12643 +10959
=============================================
+ Hits 12028 18777 +6749
- Misses 5626 58356 +52730
- Partials 829 1944 +1115
Continue to review full report at Codecov.
|
Pull Request Test Coverage Report for Build 4647
💛 - Coveralls |
|
@alex-fu Thanks for the contribution. Can you please also add an integration test for this here. Could you also add an integration test which would handle a failure and automated retry in one of the forked branches within the do_while? @manan164 We would appreciate it if you could help with reviewing this PR. |
| @@ -569,6 +569,13 @@ public WorkflowTask next(String taskReferenceName, WorkflowTask parent) { | |||
| return nextTask; | |||
| } | |||
| if (task.has(taskReferenceName)) { | |||
| if (TaskType.DO_WHILE.name().equals(task.getType())) { | |||
There was a problem hiding this comment.
@alex-fu Since we are diverging the DECISION block by so many lines of code. We can define a separate switch case for DO_WHILE task. And move common code to one function.
There was a problem hiding this comment.
OK, I'll move this common code to a function. But this function should be called from DECISION block and FORK block, not DO_WHILE block(since we don't allow nested DO_WHILE task).
| // check if already has this DO_WHILE task, ignore it if it already exists | ||
| String nextTaskReferenceName = taskToSchedule.getTaskReferenceName(); | ||
| if (workflow.getTasks().stream().anyMatch(runningTask -> runningTask.getReferenceTaskName().equals(nextTaskReferenceName))) { | ||
| return Collections.emptyList(); |
There was a problem hiding this comment.
Why we are returning an empty list. We are making get next task to return the parent DO_WHILE task let it propagates it further to decider service.
There was a problem hiding this comment.
Because in this case the parent DO_WHILE task is already exist, so we don't need to generate again. By the way, even though we return the parent DO_WHILE task, deciderService will also exclude it in below line. tasksToBeScheduled already contains this task, and putIfAbsent will not add this task again.
nextTasks.forEach(nextTask -> tasksToBeScheduled.putIfAbsent(nextTask.getReferenceTaskName(), nextTask));
|
Hi , |
|
@alex-fu Are you available to continue working on this PR? Thanks |
|
@apanicker-nflx Sorry for the late reply since I was stuck in some things last month. I'll add the integration test ASAP. |
|
Please rebase against |
|
Also, while unit tests look good, please also add an integration test for this here |
|
@apanicker-nflx I've rebased against dev and submitted some code according to @manan164's review and some integration test, please review again. |
…RK or DECISION (Netflix#1823) * 1816: getNextTask will get wrong task if DO_WHILE task is embed in FORK or DECISION * 1816: refactor WorkflowTask.next(); add integration test * 1816: refine getNextTask in WorkflowDef
Patch for #1816 .
What I changed
If the last inner task of DO_WHILE was completed and call getNextTask() in decide(), I return the DO_WHILE task (In current code, it will return the next task of DO_WHILE).
And then, in DeciderService.getNextTask(), I check if the DO_WHILE task already exists in the workflow's tasks. If it exists, just ignore it.