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

Prevent creating multiple workflow instances with the same instance id #260

Closed
Tracked by #247
cschleiden opened this issue Oct 21, 2023 Discussed in #259 · 5 comments
Closed
Tracked by #247

Prevent creating multiple workflow instances with the same instance id #260

cschleiden opened this issue Oct 21, 2023 Discussed in #259 · 5 comments
Milestone

Comments

@cschleiden
Copy link
Owner

Two subsequent CreateWorkflowInstance calls with the same id should result in an error.

Discussed in #259

@decibelcooper
Copy link
Contributor

decibelcooper commented Oct 25, 2023

thought: perhaps it's best to follow Temporal's lead and allow the user to set an ID reuse policy

A simplified binary allow/reject duplicate option should satisfy most use cases. Could probably simply be used as a hint for whether or not to generate a random execution ID? Or would it be reasonable to expose the execution ID to the client API?

EDIT: Side note: it currently feels a little awkward to serialize a workflow instance for later reference, e.g. to cancel a workflow at a later time. The reason being that the workflow instance is passed to the client API as an alias to a core struct that also includes parent/child relationship. I bring this up because if execution ID is exposed to the user in a cleaner way, it may also help the user to serialize references to workflow instances.

@decibelcooper
Copy link
Contributor

decibelcooper commented Oct 25, 2023

Another thing to note is that instance ID reuse has an impact on signaling.

Today, the behavior for signaling instances with multiple active executions is undefined at the Backend level. Looking anecdotally at the mysql backend, I see that we just snag one execution to signal. It might be reasonable to instead perform a broadcast of sorts, so that all active executions receive the signal.

@lyuboxa
Copy link

lyuboxa commented Oct 31, 2023

When I encountered this first, I looked at the potential options to enable some uniqueness in the schema without interfering too much with the codebase. Unfortunately, making instance id unique individually has implications for functionalities like ContinueAsNew where the instance id is reused with an updated execution id. This can be addressed
in code though with some safety from the database.

This led me to think that decoupling instance definition and execution may provide more flexibility.

thought: perhaps it's best to follow Temporal's lead and allow the user to set an ID reuse policy

The reuse policy definition is a good way to provide optionality, 👍

@cschleiden cschleiden added this to the 1.0 milestone Nov 12, 2023
@cschleiden
Copy link
Owner Author

Today, the behavior for signaling instances with multiple active executions is undefined at the Backend level. Looking anecdotally at the mysql backend, I see that we just snag one execution to signal. It might be reasonable to instead perform a broadcast of sorts, so that all active executions receive the signal.

There should only be one active execution at any point in time. If there are multiple, that's a bug and not as designed.

@cschleiden
Copy link
Owner Author

Closed in #288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants