-
Notifications
You must be signed in to change notification settings - Fork 52
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 multiple active instances with the same id #288
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
!bench !large !skipmysql !skipsqlite |
cschleiden
force-pushed
the
duplicate-instances
branch
from
November 17, 2023 05:34
9e4ce3a
to
310193b
Compare
!bench !large !skipmysql !skipsqlite |
Redis run
Large Redis payload run (1MB)
|
in SQLite backend
cschleiden
force-pushed
the
duplicate-instances
branch
from
November 17, 2023 18:32
c1d6df9
to
34fd5ad
Compare
cschleiden
changed the title
Draft: prevent instances with duplicate ids
Prevent multiple active instances with the same id
Nov 17, 2023
cschleiden
commented
Nov 18, 2023
cschleiden
commented
Nov 18, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this change there can only be one active workflow instance with the same ID. If one attempts to create either a workflow or sub-workflow instance with the same ID a
backend.ErrInstanceAlreadyExists
error will be returned.MySql and SQLite backends both make an additional query when creating a new workflow or sub-workflow instance.
The redis change is a bit bigger: checking for top-level instances with the same ID is relatively easy, but checking for existing subworkflow instances within the larger
CompleteWorkflowTask
transaction is not 100% safe. In practice it would probably be fine, preventing sub-workflow id collisions is relatively easy but I decided to try to move the fullCompleteWorkflowTask
transaction to a.lua
script instead. That way no other commands are executed while this is being active.The script requires quite a bit of data, so every potential key needed and every argument needs to be passed into from the calling Go code. In the script a couple helper functions
getKey()
andgetArgv()
return the next key/argument. This requires careful matching between Go and script and might be optimized by passing in a json struct instead that is then unmarshaled viacjson
in the script but this works for now.