IWF-136: Support initial data attributes when starting workflow #435
Conversation
4afa9cb to
26a0c61
Compare
integ/persistence_test.go
Outdated
| WorkflowConfigOverride: config, | ||
| UseMemoForDataAttributes: ptr.Any(useMemo), | ||
| }, | ||
| } | ||
| _, httpResp, err := reqStart.WorkflowStartRequest(wfReq).Execute() | ||
| panicAtHttpError(err, httpResp) | ||
|
|
||
| // TODO: Fix the issue with running queryHandler before new workflow which Continues as New has started |
There was a problem hiding this comment.
Looking into the code again, I now remember..
We intentionally set the query handler after the continueAsNew/dumpInternal activity. https://github.com/indeedeng/iwf/blob/main/service/interpreter/workflowImpl.go#L98
This is to ensure the correctness. If we set the query handler before that, the query handler could return empty data(since the loading hasn't completed), which will be incorrect response.
So we would rather return server errors and let the client retry later.
There was a problem hiding this comment.
Maybe you can add a comment at https://github.com/indeedeng/iwf/blob/main/service/interpreter/workflowImpl.go#L98. to remind people why this is not before https://github.com/indeedeng/iwf/blob/main/service/interpreter/workflowImpl.go#L63
There was a problem hiding this comment.
For this test, we can add a comment to explain why a sleep is necessary
integ/persistence_test.go
Outdated
| WorkflowConfigOverride: config, | ||
| UseMemoForDataAttributes: ptr.Any(useMemo), | ||
| }, | ||
| } | ||
| _, httpResp, err := reqStart.WorkflowStartRequest(wfReq).Execute() | ||
| panicAtHttpError(err, httpResp) | ||
|
|
||
| // TODO: Fix the issue with running queryHandler before new workflow which Continues as New has started | ||
| time.Sleep(time.Millisecond * 10) |
There was a problem hiding this comment.
Looks like the test failed. I looked at my other sleeps for the same reasons, I used 1 second for safety. Also it's better to only sleep for continueAsNew cases. (the test will run faster that way)
1eaec30 to
1b5d53b
Compare
integ/persistence_test.go
Outdated
| // Config is only present for continueAsNew tests | ||
| if config != nil { | ||
| // TODO: Fix the issue with running queryHandler before new workflow which Continues as New has started | ||
| time.Sleep(time.Millisecond * 1000) | ||
| } |
There was a problem hiding this comment.
Bumped the sleep to 1 second and wrapped it in a check if config is present
a530386 to
d47eb4f
Compare
| // Config is only present for continueAsNew tests | ||
| if config != nil { | ||
| for { | ||
| if err == nil || retryCount >= 5 { | ||
| break | ||
| } | ||
| // Loading data to a continuedAsNew workflow might take a few seconds thus retry mechanism is needed | ||
| time.Sleep(time.Millisecond * 1000) | ||
| retryCount += 1 | ||
| queryResult, httpResp, err = getDataAttributes(initReqQry, wfId, expectedDataAttribute, useMemo) | ||
| } | ||
| } |
There was a problem hiding this comment.
@longquanzheng what are your thoughts about this?
There was a problem hiding this comment.
That's good. I was thinking about the same way to workaround.
However, ideally we can do this in api service for everyone (that will saves all the sleeping in the tests).
But it's better to do in a separate PR (need to figure out checking error types, and backoff strategy and refactoring all the tests to remove the sleep).
Could you also raise another ticket for this?
No description provided.