Fix conflict error handling #2469
Conversation
alexshtin
force-pushed
the
feature/fix-confict-error-handling
branch
from
b19da82 to
e7a384b
Compare
| if err == consts.ErrConflict { | ||
| return false | ||
| } |
There was a problem hiding this comment.
i believe the logic should get rid of consts.ErrConflict in the long term
There was a problem hiding this comment.
There is a TODO for this (not by me), but I am not ready for this. Yet.
| if err != nil { | ||
| shard.GetLogger().Error( | ||
| "Persistent store operation Failure", | ||
| "Update workflow execution operation failed.", | ||
| tag.WorkflowNamespaceID(request.UpdateWorkflowMutation.ExecutionInfo.NamespaceId), | ||
| tag.WorkflowID(request.UpdateWorkflowMutation.ExecutionInfo.WorkflowId), | ||
| tag.WorkflowRunID(request.UpdateWorkflowMutation.ExecutionState.RunId), | ||
| tag.StoreOperationUpdateWorkflowExecution, | ||
| tag.Error(err), | ||
| ) | ||
| return nil, err | ||
| switch err.(type) { | ||
| case *persistence.CurrentWorkflowConditionFailedError, | ||
| *persistence.WorkflowConditionFailedError, | ||
| *persistence.ConditionFailedError: | ||
| // TODO get rid of ErrConflict | ||
| return nil, consts.ErrConflict | ||
| default: | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
if the intention is to emit error logs whenever error is encountered, then i would suggest log when error is generated
There was a problem hiding this comment.
I follow the rule: "Error should be propagated or handled. Not both". It gets propagated to this point and this substitution is sort of handling and good place to log it. I know we lost error call stack in this case.
|
Please update the PR description to include some context. |
| if conflictRecord["[applied]"].(bool) { | ||
| // Should never happen. All records in batch should have [applied]=false. | ||
| continue | ||
| } |
There was a problem hiding this comment.
i guess this additional check may not be necessary
What changed?
Why?
N/A
How did you test it?
Existing tests.
Potential risks
No risks.
Is hotfix candidate?
No.