Revamp retry logic #165
Revamp retry logic #165
Conversation
src/orchestrator.ts
Outdated
| let taskScheduled: TaskScheduledEvent | undefined; | ||
| let taskFailed: TaskFailedEvent | undefined; | ||
| let taskRetryTimer: TimerCreatedEvent | undefined; | ||
| for (let i = 0; i < state.length; i++) { |
There was a problem hiding this comment.
nit: can we do a foreach / for ... in-style loop here?
There was a problem hiding this comment.
I guess in TS its a for...of
There was a problem hiding this comment.
I think this was probably a case of me trying to micro-optimize the perf of this loop. Probably not worth it.
There was a problem hiding this comment.
I'm leaning away from this. I went with the explicit for-loop as we use the index in the loop body.
src/orchestrator.ts
Outdated
| let subOrchestratorCreated: SubOrchestrationInstanceCreatedEvent | undefined; | ||
| let subOrchestratorFailed: SubOrchestrationInstanceFailedEvent | undefined; | ||
| let taskRetryTimer: TimerCreatedEvent | undefined; | ||
| for (let i = 0; i < state.length; i++) { |
There was a problem hiding this comment.
same nit: for...of preferred
src/orchestrator.ts
Outdated
| const returnValue = name | ||
| ? state.filter((val: HistoryEvent) => { | ||
| return val.EventType === HistoryEventType.TaskScheduled | ||
| private findTaskScheduled(state: HistoryEvent[], name: string, startIndex: number = 0): TaskScheduledEvent | undefined { |
There was a problem hiding this comment.
Unless I'm mistaken, I don't see startIndex being passed explicitly at any point, so I'm unsure of how to use it
There was a problem hiding this comment.
I think this was a holdover from a previous iteration. I got rid of it in the rebase.
There was a problem hiding this comment.
This looks good to me, the changes are reasonable. I have a few nits that I've left as comments.
Something else I would like to see are comments in the callActivityWithRetry and callSubOrchestratorWithRetry, the methods you've modified. Just a few comments on what you're trying to achieve and how would be nice to have.
On another note, don't we call all the findTaskX (scheduled, completed, failed, etc) methods in other places as well? I wonder if we could generalize this optimization into a routine that could be re-used for those places too. In any case, if you agree, we can raise a separate issue for that.
Feel free to merge after reviewing my nits and adding some comments but let's continue our discussion on whether or not this optimization could be generalized / re-used.
The current retry logic had two flaws. First, since it restarted searching from the beginning on each attempt, it could grab the wrong scheduled activity/suborchestration on subsequent events. This could lead to incorrect results. Second, since each retry attempt started from the beginning of the state array, it was not very performant. The new approach only iterates through the state array once per call to callActivityWithRetry() or callSubOrchestrationWithRetry().
ConnorMcMahon
force-pushed
the
CleanupRetryLogic
branch
from
1720803
to
7df4804
Compare
The current retry logic had two flaws. First, since it restarted searching from the beginning on each attempt, it could grab the wrong scheduled activity/suborchestration on subsequent events. This could lead to incorrect results. Second, since each retry attempt started from the beginning of the state array, it was not very performant. The new approach only iterates through the state array once per call to callActivityWithRetry() or callSubOrchestrationWithRetry().