feat!: Restructure code in prep for vm transition #317
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
In a follow up PR we'll probably rename external dependencies to "sinks". |
Sushisource
reviewed
Oct 20, 2021
| * | ||
| * @see proposal at https://github.com/temporalio/proposals/blob/master/node/logging-and-metrics-for-user-code.md | ||
| * Dependency functions may not return values to the Workflow in order to prevent breaking determinism. |
There was a problem hiding this comment.
Suggested change
| * Dependency functions may not return values to the Workflow in order to prevent breaking determinism. | |
| * Dependency functions may not return values to the Workflow in order to help prevent breaking determinism. |
Can't actually always prevent it
There was a problem hiding this comment.
It prevents this mechanism from interfering with the Workflow execution.
Sushisource
reviewed
Oct 20, 2021
packages/worker/src/worker.ts
Outdated
| */ | ||
| maxCachedWorkflows?: number; | ||
|
|
||
| /** | ||
| * TODO: document |
Sushisource
reviewed
Oct 20, 2021
packages/worker/src/worker.ts
Outdated
Comment on lines
849
to
871
| const externalCalls = await state.workflow.getAndResetExternalCalls(); | ||
| const { dependencies } = this.options; | ||
| for (const { ifaceName, fnName, args } of externalCalls) { | ||
| const dep = dependencies?.[ifaceName]?.[fnName]; | ||
| if (dep === undefined) { | ||
| this.log.error('Workflow referenced an unregistrered external dependency', { | ||
| ifaceName, | ||
| fnName, | ||
| }); | ||
| } else if (dep.callDuringReplay || !activation.isReplaying) { | ||
| (async () => { | ||
| try { | ||
| await dep.fn({ ...state.info, isReplaying: activation.isReplaying }, ...args); | ||
| } catch (error) { | ||
| this.log.error('External dependency function threw an error', { | ||
| ifaceName, | ||
| fnName, | ||
| error, | ||
| workflowInfo: state?.info, | ||
| }); | ||
| } | ||
| })(); | ||
| } |
There was a problem hiding this comment.
Can this bit be extracted to a function w/ a docstring?
Sushisource
approved these changes
Oct 20, 2021
- Decrease Workflow bundle size from ~7.44MB to ~2.75MB - Remove otel module from `@temporalio/common` default export - Rename `WorkflowIsolateBuilder` to `WorkflowCodeBundler` and remove unused methods - Add `Workflow` and `WorkflowCreator` interfaces to support pluggable workflow environments (prepare for VM) - Simplify external dependencies mechanism to only support `void` functions and remove the `isolated-vm` transfer options.
bergundy
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@temporalio/commondefault exportWorkflowIsolateBuildertoWorkflowCodeBundlerand removeunused methods
WorkflowandWorkflowCreatorinterfaces to support pluggableworkflow environments (prepare for VM)
voidfunctions and remove the
isolated-vmtransfer options.