Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Fix case where decision task is followed by a terminate task #2056

Merged
merged 1 commit into from Jan 29, 2021

Conversation

apanicker-nflx
Copy link
Collaborator

@apanicker-nflx apanicker-nflx commented Jan 18, 2021

OLD STATE

The execution of the TERMINATE task deviated from that of other tasks such that -

  1. The task execution would modify the enclosing workflow but expect the orchestrator/state machine to persist the workflow changes
  2. In order to accomodate this behavior, the state machine evaluation had to be hacked for detection and early termination of the workflow, leading to incomplete workflow evalutions. (The most obvious side-effect of this being the TERMINATE task would be added to the workflow with a seq of 0).
  3. The incomplete state evaluation further led to the corner case such that when a DECISION task is followed by a TERMINATE task in one of its branches, the DECISION task would not be persisted by the state machine as part of the workflow data.

NEW STATE

This PR aims to resolve the above discrepancies by

  1. Making the TERMINATE task execution consistent with that of other tasks and relying on state machine to modify and update any workflow level changes.
  2. Fixing the DAG evaluation in the state machine such that it exits early when a TERMINATE task node is met in the graph.
  3. Removing the hacks during task execution and replacing it with DAG evaluation combined with workflow evaluation for early termination of workflows using the TERMINATE task.

Behavioral Impact

A behavior change for the TERMINATE task that has been introduced in this PR is that other non-terminal tasks will be marked as CANCELED keeping it consistent to the behavior if the workflow were to be TERMINATED manually (or using the API). The old behavior here was to mark such tasks as SKIPPED.

@apanicker-nflx apanicker-nflx force-pushed the terminate_within_decisino branch 3 times, most recently from f38cada to d03aba0 Compare January 19, 2021 20:15
@apanicker-nflx apanicker-nflx changed the title Terminate within decision Fix case where decision task is followed by a terminate task Jan 19, 2021
@apanicker-nflx
Copy link
Collaborator Author

apanicker-nflx commented Jan 20, 2021

@rickfish This might be of interest to you as well based on #1773 . I am currently making this change in 3.0 branch but once approved, will back-port this to 2.x as well. Please review and share your thoughts.

@rickfish
Copy link
Contributor

@apanicker-nflx Thanks for letting me know. There are so many scenarios...I will have to run my test case once the changes are in the 2.x release to see if it does indeed cancel all the tasks in all of the subworkflows and the subworkflows themselves. I am assuming that you already tested that and feel confident that the changes you made maintain the fix and also fixes other side effects.

@apanicker-nflx apanicker-nflx merged commit 5805e93 into 3.0 Jan 29, 2021
@apanicker-nflx apanicker-nflx deleted the terminate_within_decisino branch January 29, 2021 22:20
apanicker-nflx added a commit that referenced this pull request Feb 22, 2021
TwoUnderscorez pushed a commit to TwoUnderscorez/conductor that referenced this pull request Jul 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants