Update mutable state to generate workflow requests #5821
Conversation
Pull Request Test Coverage Report for Build 018eea45-745a-45b9-9dc1-c4e30fd5979fDetails
💛 - Coveralls |
Shaddoll
force-pushed
the
request
branch
6 times, most recently
from
1497967
to
5899eb4
Compare
Shaddoll
requested review from
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Shaddoll
changed the title
Shaddoll
force-pushed
the
request
branch
3 times, most recently
from
a2f6f2d
to
efb6994
Compare
| ) | ||
| if t, ok := err.(*persistence.DuplicateRequestError); ok { | ||
| if t.RequestType == persistence.WorkflowRequestTypeStart || (isSignalWithStart && t.RequestType == persistence.WorkflowRequestTypeSignal) { |
There was a problem hiding this comment.
should we check (!isSignalWithStart && t.RequestType == persistence.WorkflowRequestTypeStart) || (isSignalWithStart && t.RequestType == persistence.WorkflowRequestTypeSignal)
There was a problem hiding this comment.
no, start is included in signalwithstart request
| ) | ||
| if t, ok := err.(*persistence.DuplicateRequestError); ok { |
There was a problem hiding this comment.
nit: prefer errors.Is(err, persistence.DuplicateRequestError). That'll allow us to provide further information for the error in the future without affecting the control flow
There was a problem hiding this comment.
need type assertion, because we need to retrieve runID from the error
There was a problem hiding this comment.
errors.As would do the job then
There was a problem hiding this comment.
yes, my apologies, errors.As, though be careful to test it, it's double-pointer API is kinda confusing.
var e *persistence.DuplicateRequestError
if errors.As(err, &e) {
...
}
| @@ -3101,3 +3112,75 @@ func TestUpdateWorkflowExecutionWithNewAsPassive(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func TestValidateWorkflowRequestsAndMode(t *testing.T) { | |||
There was a problem hiding this comment.
Will you have a unit test that covers the error case in the conflict resolution flow for this change when that's updated?
What changed?
This PR is built on top of #5826.
In this PR, we generate workflow requests from external API requests and replication events and store them in database to detect duplicated requests. If a duplicated requests is detected, a DuplicateRequestError is returned by persistence layer with the run_id telling upper layer which run the request has been applied to. And when this error is returned, the API does no-op and return the run_id to the caller.
Why?
To improve idempotency of Cadence APIs
How did you test it?
unit tests
Potential risks
We have a feature flag to turn on/off this feature. And we'll rollout this feature at domain level.
Release notes
Documentation Changes