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

[Feature] Dynamically update workflows #66

Open
TaoXieSZ opened this issue Jun 24, 2023 · 8 comments
Open

[Feature] Dynamically update workflows #66

TaoXieSZ opened this issue Jun 24, 2023 · 8 comments
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@TaoXieSZ
Copy link

TaoXieSZ commented Jun 24, 2023

Hi @s8sg ,

We are using goflow to build an online system. The main use case is to orchestrate different tasks together like workflows.
For example, initially user has a workflow like

Node-A -> Node-B

But the user now want to modify and changed to

Node-A -> Node-C -> Node-B

Currently, however, after registering workflows, the DAG cannot be changed.

(Of course, we can restart the flow service to achieve the reload)

So I try to contribute in this PR #64

@s8sg s8sg added documentation Improvements or additions to documentation good first issue Good for newcomers labels Jun 24, 2023
@s8sg
Copy link
Owner

s8sg commented Jun 24, 2023

This should be already supported in GoFlow.

Register() should register the method to define the workflow. The method to define the dag executes when a request hits the go flow executor, but not at the start of the flow (at least this is what expected). This way you can dynamically change the definition of dag by changing/adding additional logic in your define workflow function

@TaoXieSZ
Copy link
Author

TaoXieSZ commented Jun 24, 2023

Hi @s8sg.

Oh, I found the Register() is very similar to the Update() I just implemented, and I can revert that change to minimize PR.

However, the root cause I implement the update logic is that I found that there will be panic in this use case:

  1. At time $t_1$, UserA called Exec() to execute FlowA, the FlowA will end at $t_3$
  2. At time $t_2$ ($t_1 < t_2 < t_3 $), UserB called Register(FlowA, AnotherHandler)

The server process will have panic.

@s8sg
Copy link
Owner

s8sg commented Jun 25, 2023

Yes. for any active request it will fail.

I think it's unavoidable. It should be taken care by the user to make sure there is no active requests

@s8sg
Copy link
Owner

s8sg commented Jun 25, 2023

Even if we keep a snapshot of the dag definition per request and use the snapshot to execute the flow

Say a dag is D with node N1, N2, N3

Say for request R1 that comes at t1 we make D1 as the snapshot as if
D1 -> N1, N2, N3

A DAG can be changed to completely remove a node. Lets say that happens at t2
D -> N1, N2

In that case for the D1 snapshot is no longer valid, and when R1 tries to execute N3 at t3 it will not find the definition

(t1<t2<t3 )

@s8sg
Copy link
Owner

s8sg commented Jun 25, 2023

Post completion #4, We should be able to do that without any concern, because workload will be completely independent of the DAG definition

Currently it's in progress. I'll suggest to wait until this feature is added

@TaoXieSZ
Copy link
Author

TaoXieSZ commented Jun 25, 2023

Even if we keep a snapshot of the dag definition per request and use the snapshot to execute the flow

Say a dag is D with node N1, N2, N3

Say for request R1 that comes at t1 we make D1 as the snapshot as if D1 -> N1, N2, N3

A DAG can be changed to completely remove a node. Lets say that happens at t2 D -> N1, N2

In that case for the D1 snapshot is no longer valid, and when R1 tries to execute N3 at t3 it will not find the definition

(t1<t2<t3 )

I think this is something like transaction concept in databases. This error is similar to read skew. I think in some cases, it is not acceptable if any running workflow is terminated accidentally by user's action. Of course, I didn't dive deep into theoretical background of workflow definition in this project.

Maybe it can be discussed further.

@TaoXieSZ
Copy link
Author

TaoXieSZ commented Jul 2, 2023

@s8sg Hi, is there any update about #4 ? That is so exciting to have a new version.

@s8sg
Copy link
Owner

s8sg commented Jul 3, 2023

Still in progress. Didn’t got much time this weekend. Will try to finish it off my the next. Will require some help with the testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants