External Storage pipeline integration#2201
Conversation
cconstable
changed the title
f5fb192 to
b0b422b
Compare
041d6f1 to
bb7a2bc
Compare
| * @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.
Added a round trip test to make sure payload in == payload out and we get a reference payload in between.
There was a problem hiding this comment.
Hmm maybe I missed it but I'm not sure I saw a round trip integration test.
I was thinking about a test where we actually run a workflow / activity, running the e2e code path.
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 ?
40ff25b to
ab3dace
Compare
@THardy98 I can add the changelog entry when the actual public interface is committed (hopefully today). |
| const { externalStorage } = this.options.loadedDataConverter; | ||
| if (externalStorage) { | ||
| await visitActivityTaskCompletion(rest, extstoreStoreOptions(externalStorage)); | ||
| } |
There was a problem hiding this comment.
For all decode paths, where external storage is not enabled, we'll want to raise an exception with TMPRL1105 and make sure it retryably fails the WFT/AT/NT (because that is correctible by enabling external storage), just like https://github.com/temporalio/sdk-python/blob/1.30.0/temporalio/converter/_data_converter.py#L323. Does not have to be part of this change though.
…aConverter. Later, we can add ExternalStorage config to DataConverter to 'enable' it.
…lly create child workflow targets.
adbe912 to
f612d67
Compare
3 participants
What was changed
LoadedDataConverter.externalStoragefield, using the payload visitorWhy?
Extstore pipeline integration sets us up for preview release.
Checklist
Closes #2202