Refactor task queue related methods of mutable state #4325
Conversation
alexshtin
force-pushed
the
feature/refactor-task-queue
branch
from
c68729b
to
8106261
Compare
| @@ -209,7 +209,7 @@ func MutableStateToGetResponse( | |||
| CurrentBranchToken: currentBranchToken, | |||
| WorkflowState: workflowState, | |||
| WorkflowStatus: workflowStatus, | |||
| IsStickyTaskQueueEnabled: mutableState.IsStickyTaskQueueEnabled(), | |||
| IsStickyTaskQueueEnabled: mutableState.TaskQueue().Kind == enumspb.TASK_QUEUE_KIND_STICKY, | |||
There was a problem hiding this comment.
This is slightly longer but also more verbose.
There was a problem hiding this comment.
This is slightly longer but also more verbose.
Wait those both mean the same thing? Also, this seems like we're doing the same thing but in a more invasive way that violates the Law of Demeter.
There was a problem hiding this comment.
I think the intent behind "more verbose" is actually "more intention revealing," which is true. MutableState.IsStickyTaskQueueEnabled could mean a number of different things. If we want to be intention-revealing and also encapsulate this comparison, I'd suggest a free function: workflow.IsSticky(*taskqueuepb.TaskQueue). Or if you're feeling fancy, workflow.IsSticky(interface { GetKind() taskqueuepb.TaskQueueKind })
There was a problem hiding this comment.
I am bringing it back under different name: IsStickyTaskQueueSet().
| workflowRunTimeout := executionInfo.WorkflowRunTimeout | ||
| taskScheduleToStartTimeout = workflowRunTimeout | ||
| } | ||
| normalTaskQueue := mutableState.GetExecutionInfo().TaskQueue |
There was a problem hiding this comment.
nit: you might use normalTaskQueueName here since it's not actually an instance of TaskQueue
| // TaskQueueScheduleToStartTimeout returns TaskQueue struct and corresponding StartToClose timeout. | ||
| // Task queue kind (sticky or normal) and timeout are set based on comparison of normal task queue name | ||
| // in mutable state and provided name. | ||
| func (ms *MutableStateImpl) TaskQueueScheduleToStartTimeout(name string) (*taskqueuepb.TaskQueue, *time.Duration) { |
There was a problem hiding this comment.
nit: I would return by value here rather than pointers but I understand that's not the common pattern.
There was a problem hiding this comment.
That's because of proto. Both return values goes to proto and there will be & operator anyway. Doing it right away simplifies caller code.
| @@ -209,7 +209,7 @@ func MutableStateToGetResponse( | |||
| CurrentBranchToken: currentBranchToken, | |||
| WorkflowState: workflowState, | |||
| WorkflowStatus: workflowStatus, | |||
| IsStickyTaskQueueEnabled: mutableState.IsStickyTaskQueueEnabled(), | |||
| IsStickyTaskQueueEnabled: mutableState.TaskQueue().Kind == enumspb.TASK_QUEUE_KIND_STICKY, | |||
There was a problem hiding this comment.
This is slightly longer but also more verbose.
Wait those both mean the same thing? Also, this seems like we're doing the same thing but in a more invasive way that violates the Law of Demeter.
| @@ -171,7 +171,7 @@ func addWorkflowTaskToMatching( | |||
| ) error { | |||
| // TODO (alex): Timeout calculation is copied from somewhere else. Extract func instead? | |||
| var taskScheduleToStartTimeout *time.Duration | |||
| if ms.IsStickyTaskQueueEnabled() { | |||
| if ms.TaskQueue().Kind == enumspb.TASK_QUEUE_KIND_STICKY { | |||
There was a problem hiding this comment.
I'm really not sure why we're inlining this
| // If current task queue becomes sticky in between when this transfer task is created and processed, | ||
| // it can't be used at process time, because timeout timer was not created for it, | ||
| // because it used to be non-sticky when this transfer task was created here. | ||
| // In short, task queue that was "current" when transfer task was created must be used when task is processed. |
There was a problem hiding this comment.
To keep things DRY, I'd recommend just using a godoc doc link to a canonical source on this non-sticky->sticky timeout issue i.e.. [package.Struct.Method]. It's here under "Doc links"
There was a problem hiding this comment.
It is slightly different here. And I don't think that doc links work inside functions.
What changed?
Refactor task queue related methods of mutable state:
ClearStickyness()->ClearStickyTaskQueue(),IsStickyTaskQueueEnabled()->IsStickyTaskQueueSet(),TaskQueue()->CurrentTaskQueue().SetStickyTaskQueue(),TaskQueueScheduleToStartTimeout().Improved comments.
Why?
To concolidate logic related to timeout calulation.
Cleaner contract between mutable state and its consumers.
How did you test it?
Existing tests.
Potential risks
No risks.
Is hotfix candidate?
No.