External Storage pipeline integration#2201
Conversation
cconstable
changed the title
f5fb192 to
b0b422b
Compare
…aConverter. Later, we can add ExternalStorage config to DataConverter to 'enable' it.
041d6f1 to
bb7a2bc
Compare
| } = {} | ||
| ): Promise<void> { | ||
| const runner = new ExternalStorageRunner(externalStorage); | ||
| return visitWorkflowActivationCompletion(completion, { |
There was a problem hiding this comment.
This needs to provide different storage contexts for each command within the workflow task completion. See https://github.com/temporalio/sdk-python/blob/1.30.0/temporalio/worker/_workflow_instance.py#L2302 in Python.
| * @internal | ||
| * @experimental | ||
| */ | ||
| export function storeWorkflowActivationCompletion( |
There was a problem hiding this comment.
Need analogous functions for activity activation and completion.
| * @internal | ||
| * @experimental | ||
| */ | ||
| export function storeWorkflowActivationCompletion( |
There was a problem hiding this comment.
Maybe similarly, we need to update the client call sites to store and retrieve as well. Could use the visitor or invoke per-field; my preference is to use the visitor so we don't have to remember to add more specialized calls for new fields.
| t.deepEqual(args[1], small); | ||
| }); | ||
|
|
||
| test('store then retrieve round-trips the original payload bytes through a worker message', async (t) => { |
There was a problem hiding this comment.
Might be nice to have an integration test that does a round-trip, where we don't manually construct the activation/completion ourselves.
There was a problem hiding this comment.
Also would be nice to add equivalent cases for activity payloads that exceed the external storage payload threshold (unsure if this was intended to be part of the change or if this PR was scoped to workflows)
There was a problem hiding this comment.
LGTM, would prefer some additional test coverage though (noted in comment)
probably prudent to get approval from @jmaeagle99 for ext storage API semantics
THardy98
left a comment
THardy98
left a comment
There was a problem hiding this comment.
should we add to CHANGELOG.MD ?
3 participants
What was changed
externalStoragetoLoadedDataConverterso we can plug it into the process activation pipeline.WorkflowCodecRunner.encodeCompletionto just encode instead of encoding AND applyingcoresdk.workflow_completion.WorkflowActivationCompletion.encodeDelimited(encodedCompletion).finish();. This allows us to encode then store and then callencodeDelimitedafter extstore.Why?
Extstore pipeline integration sets us up for preview release.
Checklist
Closes #2202