Eager Workflow Start #1757
Eager Workflow Start #1757
Conversation
THardy98
force-pushed
the
eager_workflow_start
branch
from
24a1314 to
9d137ac
Compare
|
|
||
| const start = composeInterceptors(interceptors, 'start', this._startWorkflowHandler.bind(this)); | ||
| // Adapt legacy `start` interceptors to the new `startWithDetails` interface. | ||
| const adaptedInterceptors: WorkflowClientInterceptor[] = interceptors.map((i) => { |
There was a problem hiding this comment.
Nits:
- Should probably move these adaptors to a distinct file. The
workflow-client.tsfile is already quite heavy. - I think we should adapt interceptors on Client creation, so that we don't have to do this repeatedly here.
There was a problem hiding this comment.
moved to interceptor-adapters.ts
I tried moving the adapt interceptors logic to client creation, but it causes an otel interceptor test to fail (Otel interceptor spans are connected and complete). I spent probably too much time trying to debug this (I believe it has to do with losing this context when we try to transform the interceptor), but wasn't able to solve it without resorting back to does this transformation directly in _start
There was a problem hiding this comment.
Just had a very quick look. Deferring to @Sushisource for more complete check.
| // Apply adapters to workflow client interceptor. | ||
| export function adaptWorkflowClientInterceptor(i: WorkflowClientInterceptor): WorkflowClientInterceptor { |
There was a problem hiding this comment.
Comment mostly just repeats function name. The comment for the "real" function seems sufficient.
packages/client/src/interceptors.ts
Outdated
| * Successor to {@link start}. Unlike {@link start}, this method returns | ||
| * start details via {@link WorkflowStartOutput}. |
There was a problem hiding this comment.
This comment will need to change when the deprecated start is removed. I'd get rid of the reference to the old thing and just talk about what it does.
| const interceptors = Array.isArray(this.options.interceptors) | ||
| ? (this.options.interceptors as WorkflowClientInterceptor[]) | ||
| : []; | ||
| // Apply adapters to workflow client interceptors. |
| * Potentially reduce the latency to start this workflow by encouraging the server to | ||
| * start it on a local worker running with this same client. |
There was a problem hiding this comment.
| * Potentially reduce the latency to start this workflow by encouraging the server to | |
| * start it on a local worker running with this same client. | |
| * Potentially reduce the latency to start this workflow by requesting that the server | |
| * start it on a local worker running with this same client. |
- add startWithDetails to WorkflowClientInterceptor to support return type of WorkflowStartOutput, inject if not defined at _start call
- symbol usage to mark if connection supports ews - separate file for interceptor adapters, adapt wf client interceptors on wf client creation - deprecate `start` wf client interceptor in favor of `startWithDetails`
THardy98
force-pushed
the
eager_workflow_start
branch
from
9638f7f to
33d672c
Compare
What was changed
The main change to accommodate eager workflow start was adding
startWithDetailstoWorkflowClientInterceptorto support a (new) return type ofWorkflowStartOutput. This allows us to return an object, giving us the ability to add new fields without issue - we addeagerlyStartedin this PR. This new interceptor method is injected if not defined at the internal_startcall, and adapts the existingstartinterceptor method to usestartWithDetails. We also define a new handle calledWorkflowHandleWithStartDetailswhich extends the existingWorkflowHandleWithFirstExecutionRunId, which now includes whether the workflow was started eagerly or not. Adding fields to the handle is an adopted pattern in the TS SDK.Why?
Support eager workflow start.
Closes [Feature Request] Eager Workflow Start #1348
How was this tested:
Existing integration tests and 2 new integration tests.
Any docs updates needed?
Probably