Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documentation about workflow and task lifecycle events #1054

Merged

Conversation

cdavernas
Copy link
Member

@cdavernas cdavernas commented Jan 9, 2025

Please specify parts of this PR update:

  • Specification
  • Schema
  • Examples
  • Extensions
  • Use Cases
  • Community
  • CTK
  • Other

Discussion or Issue link:
#1024
#1030

What this PR does:

  • Adds documentation about workflow and task lifecycle events

@cdavernas cdavernas added change: documentation Improvements or additions to documentation. It won't impact a version change. change: feature New feature or request. Impacts in a minor version change area: spec Changes in the Specification labels Jan 9, 2025
@cdavernas cdavernas added this to the v1.0.0 milestone Jan 9, 2025
@cdavernas cdavernas self-assigned this Jan 9, 2025
@cdavernas cdavernas linked an issue Jan 9, 2025 that may be closed by this pull request
@cdavernas cdavernas linked an issue Jan 9, 2025 that may be closed by this pull request
+ [Workflow Completed](#workflow-completed-event)
+ [Workflow Status Changed](#workflow-status-changed-event)
- [Task Lifecycle Events](#task-lifecycle-events)
+ [Task Created](#task-created-event)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see how a task can be created and not started

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, a task can be instantiated, meaning initialized and provisioned with its input, and not have yet started. That's what we do in Synapse. Unlike workflows, tasks do not exist beforehand, you therefore need to tell the user about them being created: you cannot start something that has not been created.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets use an example
do

  • firstTask
  • secondTask
  • thirdTask

I guess the sequence of events will be firstTask created, first task started, ..., first task completed, second task created, second task started, ..., second task completed

so inmediatealy after firstTask instance has been created with some input, it will start execution. (obviously you have to create something before executing it, which Im discussing is the fact that a task can be created without starting it just inmediately)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What Im trying to say is the same we do not have an event called "setting input to task instance before actually starting it" we do not need an event "taks created before starting it"

Copy link
Collaborator

@fjtirado fjtirado Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im trying to save a redundant event, thats it, but if you feel the distinction between task created and task started is relevant, as far as it not fordibben for them to be simoulteneous in a particular implementation, Im fine

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JBBianchi Thats it. For me the moment in which is a task instance is created is an implementation detail the user is not really interested on, specially when, as far as I know, we do not really have the concept of an schedule task and users cannot arbitrarily exeucte task out of order (the task are executed according to the workflow definition) or do not explicitly intervene in the transitions (they might indirectly intervene by sending events for listen task or in custom ones, but there is not a task phase transition API the user might invoke. In other words, task are not created waiting for the user to start them manually. If that was the situation, the event will make perferct sense, but that being not the case, this state creates some ambiguity that I will prefer to avoid)

Copy link
Member Author

@cdavernas cdavernas Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fjtirado The problem that I see is the data carried by the created event, which I do not want to carry over to the started one, for semantic reasons. While I do not care of getting rid of the created event, I am strongly opposed to moving its payload to the started one. However, not doing so, we are forbidding the external world to know intrinsics about the task being run.

Copy link
Member Author

@cdavernas cdavernas Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, task are not created waiting for the user to start them manually.

@fjtirado No, but as @JBBianchi mentionned, you could have provisionned them beforehand, before running them in the order defined by the workflow, in a warmup procedure or whatever.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we define the required ones in the spec and allow implementations to emit others as they find specific use cases? Later, if the proposal is compelling and the community finds it useful, we can add it to the spec.

This discussion is endless.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that's exactly what @fjtirado and I concluded hours ago in Slack PMs 😉

Copy link
Collaborator

@fjtirado fjtirado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have several doubts about the concept of task life cycle.
Its doable, buy pretty tricky and Im not sure we have to force implementors to keep track of every task status.
Also, for some task, for example, wait, which should be the status? suspended?

dsl-reference.md Outdated Show resolved Hide resolved
dsl-reference.md Outdated Show resolved Hide resolved
dsl-reference.md Outdated Show resolved Hide resolved
dsl-reference.md Outdated Show resolved Hide resolved
dsl-reference.md Outdated Show resolved Hide resolved
dsl-reference.md Outdated Show resolved Hide resolved
dsl-reference.md Outdated Show resolved Hide resolved
dsl-reference.md Outdated Show resolved Hide resolved
dsl-reference.md Outdated Show resolved Hide resolved
dsl.md Show resolved Hide resolved
@cdavernas
Copy link
Member Author

cdavernas commented Jan 9, 2025

Also, for some task, for example, wait, which should be the status? suspended?

@fjtirado That's specified in the dsl.md file:

| waiting | The workflow/task execution is temporarily paused, awaiting either inbound event(s) or a specified time interval as defined by a wait task. |

cdavernas and others added 9 commits January 9, 2025 16:26
Co-authored-by: Ricardo Zanini <[email protected]>
Co-authored-by: Ricardo Zanini <[email protected]>
Co-authored-by: Ricardo Zanini <[email protected]>
Co-authored-by: Ricardo Zanini <[email protected]>
Co-authored-by: Ricardo Zanini <[email protected]>
Co-authored-by: Ricardo Zanini <[email protected]>
Co-authored-by: Ricardo Zanini <[email protected]>
Co-authored-by: Ricardo Zanini <[email protected]>
Co-authored-by: Ricardo Zanini <[email protected]>
@cdavernas
Copy link
Member Author

I have several doubts about the concept of task life cycle.
Its doable, buy pretty tricky

I must be missing something, because I do not see the problem with it. As a matter of fact, we are already doing it in Synapse, as you can see here, for example.

Copy link
Collaborator

@fjtirado fjtirado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets perform the agreed changed and move forward
@cdavernas Thanks a lot!

@cdavernas cdavernas requested a review from fjtirado January 27, 2025 11:02
@cdavernas cdavernas merged commit 5a68140 into serverlessworkflow:main Jan 27, 2025
3 checks passed
@cdavernas cdavernas deleted the feat-lifecycle-cloud-events branch January 27, 2025 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: spec Changes in the Specification change: documentation Improvements or additions to documentation. It won't impact a version change. change: feature New feature or request. Impacts in a minor version change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Cloud events to be publlished when workflow status change Add a new suspended workflow status phase
4 participants