Refactor failWorkflowTask method #3915
Conversation
alexshtin
force-pushed
the
refactor/fail-wt
branch
from
ce06b47 to
0ce328b
Compare
| return handler.failWorkflowTaskOnInvalidArgument(validationFn()) | ||
| } | ||
|
|
||
| func (handler *workflowTaskHandlerImpl) failWorkflowTaskOnInvalidArgument( |
There was a problem hiding this comment.
why we need this method? can we just call failWorkflowTask(wtFailedCause, err) in places where you need this method?
There was a problem hiding this comment.
I think it's to distinguish an error from user input vs an internal error?
cc: @alexshtin
There was a problem hiding this comment.
I don't want to change any logic in "refactor" style PR. But yes, having both of them is not right. I was thinking about reverse approach: calling failWorkflowTaskOnInvalidArgument everywhere where failWorkflowTask is called now. Wrong "user" input should fail WT but any server errors should not. I need to track all possible errors here to make a call and it is out of scope for this small refactoring.
| return handler.failWorkflowTaskOnInvalidArgument(validationFn()) | ||
| } | ||
|
|
||
| func (handler *workflowTaskHandlerImpl) failWorkflowTaskOnInvalidArgument( |
There was a problem hiding this comment.
I think it's to distinguish an error from user input vs an internal error?
cc: @alexshtin
| wtFailedCause = NewWorkflowTaskFailedCause(enumspb.WORKFLOW_TASK_FAILED_CAUSE_BAD_BINARY, serviceerror.NewInvalidArgument(fmt.Sprintf("binary %v is already marked as bad deployment", request.GetBinaryChecksum()))) | ||
| wtFailedCause = newWorkflowTaskFailedCause( | ||
| enumspb.WORKFLOW_TASK_FAILED_CAUSE_BAD_BINARY, | ||
| serviceerror.NewInvalidArgument(fmt.Sprintf("binary %v is already marked as bad deployment", request.GetBinaryChecksum())), |
There was a problem hiding this comment.
| serviceerror.NewInvalidArgument(fmt.Sprintf("binary %v is already marked as bad deployment", request.GetBinaryChecksum())), | |
| serviceerror.NewInvalidArgument( | |
| fmt.Sprintf( | |
| "binary %v is already marked as bad deployment", | |
| request.GetBinaryChecksum(), | |
| ), | |
| ), |
There was a problem hiding this comment.
and I believe word "already" doesn't belong here.
alexshtin
force-pushed
the
refactor/fail-wt
branch
from
0ce328b to
07fd32e
Compare
Not in this one. Stay tuned :-) |
alexshtin
force-pushed
the
refactor/fail-wt
branch
from
07fd32e to
94ac913
Compare
What changed?
Rename
failCommandtofailWorkflowTaskbecause it fails workflow task not command and extract other methods to better code reuse.Why?
To reuse code to fail workflow task in different places.
How did you test it?
Existing tests.
Potential risks
No risks.
Is hotfix candidate?
No.