signals(dedup): deduplicate signals and ensure all signals received #205
Conversation
| // signal de-duplication fields | ||
| let expectedSignalCount = 0; | ||
| const expectedSignalIds = new Set<number>(); | ||
| const receivedSignalIds = new Set<number>(); |
There was a problem hiding this comment.
How are you transferring these on continue-as-new?
There was a problem hiding this comment.
Thinking out out loud: what if we always fed the workflowState into the new workflow? The CAN action has "arguments" that are propagated right now.
| WorkflowInput: &WorkflowInput{ | ||
| ExpectedSignalCount: 10, | ||
| InitialActions: ListActionSet( | ||
| NewAwaitWorkflowStateAction("signals_complete", "true"), |
There was a problem hiding this comment.
This might be too brittle since NewSetWorkflowStateAction can override that entire state if I'm not mistaken. It would be better to safe-guard that special key somehow.
There was a problem hiding this comment.
Related to that, I've been wondering if SetWorkflowStateAction overriding the entire map is helpful. If it only updated a single key, that might be more useful.
Or we add a new MergeWorkflowStateAction that ... well merges selectively. But that would still be vulnerable to overrides from SetWorkflowStateAction.
|
|
||
| // Check if all expected signals have been received (only for signals with IDs) | ||
| if receivedID != 0 && state.validateSignalCompletion() { | ||
| state.workflowState.Kvs["signals_complete"] = "true" |
There was a problem hiding this comment.
Do you even need this in the workflow state kvs?
The workflow should just fail if it did not receive all of the expected signals.
There was a problem hiding this comment.
Still don't understand how the list of expected signals would be carried over the continue as new boundary.
| ClientSequence: &ClientSequence{ | ||
| ActionSets: []*ClientActionSet{ | ||
| { | ||
| Actions: NewSignalActionsWithIDs(1, 2, 3, 4, 5, 6, 7, 8, 9, 10), |
There was a problem hiding this comment.
Probably best to just give a range here instead of having to provide each individual signal ID.
spkane31
force-pushed
the
spk/count-dedup-signals
branch
from
b51756f to
7b71342
Compare
What was changed
Add # of signals as a workflow input
Why?
To ensure expected number of unique signals is accurate and no action from signals that are executed more than once.
Checklist
Closes [Feature Request] Count and de-duplicate signals in kitchensink #204
How was this tested: added unit tests
Any docs updates needed?