RespondWorkflowTaskCompleted returns error on bad commands #655
Conversation
alexshtin
requested review from
mfateev and
samarabbas
and removed request for
mfateev
| return []*commandpb.Command{ | ||
| { | ||
| CommandType: enumspb.COMMAND_TYPE_COMPLETE_WORKFLOW_EXECUTION, | ||
| CommandType: enumspb.COMMAND_TYPE_START_TIMER, |
There was a problem hiding this comment.
This is 🤦. With new handler logic, I was able to catch it.
| }, | ||
| }) | ||
| s.Nil(err, s.printHistory(msBuilder)) | ||
| executionBuilder := s.getBuilder(testNamespaceID, we) |
There was a problem hiding this comment.
s.getBuilder doesn't work anymore, because cache is invalidated and there is no ExecutionInfo. Instead, I am using ms2 object for assertes in all unit tests. I believe ms2 is a mutable state that actually get mutated with last RespondWorkflowTaskCompleted API call.
| failWorkflowTaskInfo struct { | ||
| cause enumspb.WorkflowTaskFailedCause | ||
| message string | ||
| workflowTaskFailedError struct { |
There was a problem hiding this comment.
Did some minor refactoring here.
| } | ||
|
|
||
| if workflowTaskFailedErr != nil { | ||
| switch workflowTaskFailedErr.causeErr.(type) { |
There was a problem hiding this comment.
I don't like that we have logic about what error to return back to the caller spread across multiple files. I think we should have the workflow task error logic only within workflowTaskHandler.go.
samarabbas
left a comment
samarabbas
left a comment
There was a problem hiding this comment.
Can you please address my comments.
service/history/historyEngine.go
Outdated
|
|
||
| if _, err = mutableState.AddWorkflowTaskFailedEvent( | ||
| scheduleID, startedID, cause, failure, request.GetIdentity(), request.GetBinaryChecksum(), "", "", 0, | ||
| scheduleID, startedID, workflowTaskFailedErr.failedCause, failure.NewServerFailure(workflowTaskFailedErr.Error(), true), request.GetIdentity(), request.GetBinaryChecksum(), "", "", 0, |
There was a problem hiding this comment.
Why are we wrapping it in NewServerFailure?
There was a problem hiding this comment.
Maybe we can add another method on workflowTaskFailedErr type to convert it into right format to add an history event for it.
There was a problem hiding this comment.
WorkflowTaskFailedEvent has failure field which may contains different failures. Here it is just error message but other places may set other failures. The best way to refactor this, is to replace workflowTaskFailedError with WorkflowTaskFailure which will contains failedCause enum and error message. I don't know if it worth it. I would leave it as is for now.
samarabbas
left a comment
samarabbas
left a comment
There was a problem hiding this comment.
Looks good. You can land it after addressing my comment.
2 participants
What changed?
If
failWorkflowTaskwas created incompleteWorkflowTaskthen entire request fails.Why?
Fixes #652.
How did you test it?
Added integration test.
Potential risks
If anyone relies on success return in case of malformed request (very unlikely), their code will be broken.