Track mutable side effect calls #1051
Conversation
Quinn-With-Two-Ns
force-pushed
the
mutable-side-effect-counter
branch
from
83c74ff to
f242d79
Compare
|
Marking as a draft because I manly want feedback on the counting approach |
|
Discussed offline and agreed to leave old broken workflow behavior as is. I will need to add some tests for this. |
|
👍 Will wait for ready-to-review before reviewing in earnest |
Quinn-With-Two-Ns
force-pushed
the
mutable-side-effect-counter
branch
2 times, most recently
from
49785de to
d8c78af
Compare
worker/worker.go
Outdated
| // WorkflowID is optional and defaults to ReplayId. | ||
| // | ||
| // Note: For all replay functions if no WorkflowID is given for replay the default is ReplayId. | ||
| GetWorkflowResult(workflowID string, valuePtr interface{}) error |
There was a problem hiding this comment.
We should have added something like this when we stopped checking the workflow execution result and we need it to test we didn't break old behaviour.
| if ok { | ||
| err = weh.dataConverter.FromPayloads(counterHintPayload, &counterHint) | ||
| } else { | ||
| // An old version of the SDK did not write the counter hint so we have to assume. |
There was a problem hiding this comment.
Maybe if we detect this scenario we should not write counterHints to maintain behaviour in a given workflow?
There was a problem hiding this comment.
I struggle to follow this code well enough to know, but so long as old behavior remains, any approach is fine by me
Quinn-With-Two-Ns
force-pushed
the
mutable-side-effect-counter
branch
from
d8c78af to
43d30ee
Compare
internal/internal_event_handlers.go
Outdated
| @@ -782,15 +788,51 @@ func (wc *workflowEnvironmentImpl) SideEffect(f func() (*commonpb.Payloads, erro | |||
| wc.logger.Debug("SideEffect Marker added", tagSideEffectID, sideEffectID) | |||
| } | |||
|
|
|||
| func (wc *workflowEnvironmentImpl) lookupMutableSideEffect(id string) (*commonpb.Payloads, bool) { | |||
There was a problem hiding this comment.
May need a comment. I am confused what this is doing at first glance. It's a mutable lookup?
| if ok { | ||
| err = weh.dataConverter.FromPayloads(counterHintPayload, &counterHint) | ||
| } else { | ||
| // An old version of the SDK did not write the counter hint so we have to assume. |
There was a problem hiding this comment.
I struggle to follow this code well enough to know, but so long as old behavior remains, any approach is fine by me
| @@ -217,3 +217,39 @@ func SideEffectWorkflow(ctx workflow.Context, field string) error { | |||
| func EmptyWorkflow(ctx workflow.Context, _ string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func MutableSideEffectWorkflow(ctx workflow.Context) ([]int, error) { | |||
There was a problem hiding this comment.
Is it possible, maybe in addition to these replay tests, to replicate the error that caused this issue in the first place? Maybe this workflow will, haven't tested, but want to make sure we can verify new behavior is for new workflow run against server. Or is that what that small assertion addition in workflow_test.go already does?
Also I wonder if we want to have an integration test proving non-deterministic mutable side effect order change. That may be asking too much though.
There was a problem hiding this comment.
Or is that what that small assertion addition in workflow_test.go already does?
Yes exactly
Also I wonder if we want to have an integration test proving non-deterministic mutable side effect order change. That may be asking too much though.
not sure I understand, do you mean a test that shows changing the order of mutable side effects is not deterministic?
There was a problem hiding this comment.
do you mean a test that shows changing the order of mutable side effects is not deterministic?
I do. Basically if I add a mutable side effect for the ID if it's non-deterministic. But I think this is a bit much, no need.
Track how many times a mutable side effect is called to distinguish multiple
MutableSIdeEffectcalls in the same WFT.I'll note this PR wont help existing histories that executed multiple
MutableSIdeEffectin the same WFT. If we also use theCommandIDto help distinguish multiple calls. The could help certain histories replay correctly. The trade off is an even more complex implementation. There will always be some histories that we cannot save because the SDK did not write enough information to historyResolves: #1014