Extract workflow execution delete code to workflow.DeleteManager #2388
Conversation
| DeleteDeletedWorkflowExecution(namespaceID namespace.ID, we commonpb.WorkflowExecution, weCtx Context, ms MutableState, transferTaskVersion int64) error | ||
| DeleteWorkflowExecutionRetention(namespaceID namespace.ID, we commonpb.WorkflowExecution, weCtx Context, ms MutableState, timerTaskVersion int64) error |
There was a problem hiding this comment.
DeleteDeleted? What does that mean? Could you put some comment on them, what are the differences of these 2 APIs.
And, ctx is usually first input argument.
There was a problem hiding this comment.
+1. DeleteDeleted will be used by transfer queue I guess?
nit : DeleteForWorkflowRetention makes more sense to me but no strong opinion.
Also, probably do not reference any queue type (transfer, time) in new code as we are doing the refactoring for queue processors. Maybe taskVersion is sufficient in this case.
There was a problem hiding this comment.
This PR was cherry-picked from another bigger PR. DeleteDeleted makes perfect sense there but here it looks confusing, I agree. I am renaming it. At least for now.
This is workflow.Context not context.Context, so I believe it is ok it pass it with other workflow related params.
Renaming DeleteWorkflowExecutionRetention to DeleteWorkflowExecutionByRetention.
And also giving task version parameters more generic names.
| @@ -60,12 +60,14 @@ type ( | |||
| matchingClient matchingservice.MatchingServiceClient | |||
| config *configs.Config | |||
| searchAttributesProvider searchattribute.Provider | |||
| workflowDeleteManager workflow.DeleteManager | |||
There was a problem hiding this comment.
looks like it's not used? Do we plan to use it in the future?
There was a problem hiding this comment.
Yes, this will be used in next PR.
| @@ -99,9 +95,6 @@ func (t *timerQueueTaskExecutorBase) executeDeleteHistoryEventTask( | |||
| if err != nil { | |||
| return err | |||
| } | |||
| if mutableState == nil || mutableState.IsWorkflowExecutionRunning() { | |||
There was a problem hiding this comment.
we still need to check if mutableState is nil or not here I think. Otherwise the GetLastWriteVersion call may panic on L106.
There was a problem hiding this comment.
loadMutableStateForTimerTask never returns nil mutable state and nil error. If there is an error it will be handled, if not, it can go through.
There was a problem hiding this comment.
but don't you need to make sure workflow is not running? We don't want to delete history for running workflow.
There was a problem hiding this comment.
I tried to move as much logic as possible to deleteManager. So this check is there now.
alexshtin
force-pushed
the
feature/extract-delete-manager
branch
from
f5016f2 to
b08afa1
Compare
| @@ -99,9 +95,6 @@ func (t *timerQueueTaskExecutorBase) executeDeleteHistoryEventTask( | |||
| if err != nil { | |||
| return err | |||
| } | |||
| if mutableState == nil || mutableState.IsWorkflowExecutionRunning() { | |||
There was a problem hiding this comment.
but don't you need to make sure workflow is not running? We don't want to delete history for running workflow.
| return deleteManager | ||
| } | ||
|
|
||
| func (m *DeleteManagerImpl) DeleteWorkflowExecution( |
There was a problem hiding this comment.
so the difference from DeleteWorkflowExecutionByRetention is this method allow delete running workflow? Why do we need to delete running workflow? Isn't that we require workflow to be closed before you can delete them?
There was a problem hiding this comment.
Because of cherry-picking I missed check for running workflow. Restored.
What changed?
Extract workflow execution delete code to
workflow.DeleteManager.Why?
Refactoring for future code reuse.
How did you test it?
Modified existing tests.
Potential risks
No risks.
Is hotfix candidate?
No.