add asyncComplete support to sdk Task base class - #136
Conversation
asyncComplete was present on WorkflowTask but not exposed through the sdk, making it impossible to create async http or event tasks without subclassing as a workaround. adds the field to Task<T> alongside optional and startDelay, wires it through toWorkflowTask() and restores it from the WorkflowTask constructor. any task type can now call .asyncComplete(true). Signed-off-by: klouds27 <adalwolf@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
v1r3n
left a comment
v1r3n
left a comment
There was a problem hiding this comment.
Looks good. The base task builder now preserves asyncComplete when converting both to and from WorkflowTask, and the added round-trip test verifies the public HTTP task path. I found no correctness, security, or test-coverage issue requiring changes.
|
@klouds27 make it ready for review when ready. |
There was a problem hiding this comment.
🟡 Not ready to approve
The Task(WorkflowTask) constructor still drops existing base fields (startDelay, optional) when rebuilding tasks via TaskRegistry, which breaks conversion round-trips in the same code path being updated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Exposes asyncComplete on the SDK Task<T> base class (previously only available on WorkflowTask), enabling async HTTP/event-style tasks without requiring task subclassing workarounds.
Changes:
- Add
asyncCompletestate + fluent setter/getter toTask<T>. - Wire
asyncCompletethroughTask.toWorkflowTask()and restore it when constructing aTaskfrom aWorkflowTask. - Add a conversion test to validate
Httptask asyncComplete round-tripping throughWorkflowTask.
File summaries
| File | Description |
|---|---|
| conductor-client/src/main/java/com/netflix/conductor/sdk/workflow/def/tasks/Task.java | Adds asyncComplete support and maps it to/from WorkflowTask. |
| conductor-client/src/test/java/com/netflix/conductor/sdk/workflow/def/TaskConversionsTests.java | Adds coverage to ensure asyncComplete is preserved across conversions for Http. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| protected Task(WorkflowTask workflowTask) { | ||
| this(workflowTask.getTaskReferenceName(), TaskType.valueOf(workflowTask.getType())); | ||
| this.input = workflowTask.getInputParameters(); | ||
| this.description = workflowTask.getDescription(); | ||
| this.name = workflowTask.getName(); | ||
| this.asyncComplete = Boolean.TRUE.equals(workflowTask.isAsyncComplete()); | ||
| } |
asyncCompleteexists onWorkflowTaskbut is not exposed through the sdk, making it impossible to define async http or event tasks without subclassing as a workaround.adds the field to
Task<T>alongsideoptionalandstartDelay, wires it throughtoWorkflowTask()and restores it from theWorkflowTaskconstructor. any task type can now call.asyncComplete(true).