Fix rerunWorkflow places synchronous system tasks in the queue #2494
Conversation
| @@ -58,7 +59,7 @@ public JsonJqTransform(ObjectMapper objectMapper) { | |||
| @Override | |||
| public void start(Workflow workflow, Task task, WorkflowExecutor executor) { | |||
| final Map<String, Object> taskInput = task.getInputData(); | |||
| final Map<String, Object> taskOutput = task.getOutputData(); | |||
| final Map<String, Object> taskOutput = task.getOutputData() != null ? task.getOutputData(): new HashMap<>(); | |||
There was a problem hiding this comment.
getOutputData() does not return null. Can you please explain the need for a null check here?
There was a problem hiding this comment.
The outputData field is reset to null here: https://github.com/Netflix/conductor/blob/main/core/src/main/java/com/netflix/conductor/core/execution/WorkflowExecutor.java#L1642 I guess we could update that line to just clear the map instead of setting it null, but feel there might be other cases that I might neglected if I changed that. So I added the null check for outputData in the system task instead. And that's the only sync system task that need to add the null check
There was a problem hiding this comment.
It makes sense to clear the outputData or set it to an empty HashMap during rerun. If you look at the Task, the outputData field is defaulted to an empty map. So it's natural for the rest of the code to expect a non-null value from Task.getOutputData(). DoWhile also expects a non-null output data and would fail if null is returned.
Also, the fewer nulls we introduce, the less chance of an unexpected NullPointerException.
There was a problem hiding this comment.
ya updated it to clear the outputData instead of setting to null
| @@ -1324,6 +1340,59 @@ public void testRerunWorkflowWithTaskId() { | |||
| assertEquals(new HashSet<>(), workflow.getFailedReferenceTaskNames()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testRerunWorkflowWithSyncSystemTaskId() { | |||
| tasks.size() == 1 | ||
| tasks[0].status == Task.Status.FAILED | ||
| tasks[0].taskType == 'JSON_JQ_TRANSFORM' | ||
| tasks[0].reasonForIncompletion as String == "Cannot index string with string \"array\"" |
There was a problem hiding this comment.
The as String may not be necessary. Also, using single quotes ' ensures that Groovy uses a String and not a GString.
| tasks.size() == 1 | ||
| tasks[0].status == Task.Status.COMPLETED | ||
| tasks[0].taskType == 'JSON_JQ_TRANSFORM' | ||
| tasks[0].outputData as String == "[result:[out:[a, b, c, d]], resultList:[[out:[a, b, c, d]]]]" |
There was a problem hiding this comment.
updated as well, the as String is necessary for outputData though as it's a map, so kept it, but used single quotes for the match string instead
There was a problem hiding this comment.
I missed that it's a map here. In that case, it makes sense to assert the keys of the map rather than converting it to s String and asserting that. The assertion will fail, if the toString() logic in Map changes, its unlikely, but possible.
There was a problem hiding this comment.
I think we can rely on Java's implementation of toSTring() of the Map. It's kind of cumbersome to write a loop through the map to assert each key/value pair. What do you think?
There was a problem hiding this comment.
Verifying the contents of a Map using its toString() is not ideal. It's also harder to understand and refactor. The goal of the test is not to verify if jackson-jq is doing its work, so you could
- write a simpler
jqexpression that is easy to verify or - remove the
outputDataassertion altogether or - assert that
output.result != nullandoutput.resultList != null
There was a problem hiding this comment.
Make sense, didn't realize the outputData result is JsonNode. yea, the purpose of the test isn't for those kind of checks. Refactored to only do basic assertions.
- clear task output on rerun instead of set it to null - refactor outputData checks for JsonJQTransformSpec
Pull Request type
Changes in this PR
When rerun from task, if it's a synchronous system task, don't put it in the queue, start directly instead.
Describe the new behavior from this PR, and why it's needed
Issue #MWI-3456: rerunWorkflow places synchronous system tasks in the queue
Alternatives considered
Describe alternative implementation you have considered