Repository navigation
Add transfer type converters - #3073
VegetarianOrc wants to merge 19 commits into
Conversation
| // Set merged plugins after configuration, then build | ||
| builder.setPlugins(mergedPlugins); | ||
| options = builder.build(); | ||
| options = |
There was a problem hiding this comment.
Why build the options then immediately call ScheduleClientOptions.newBuilder(options)?
There was a problem hiding this comment.
I took a pass at cleaning this up a little by passing the data converter into the invoker rather than pulling it from the options. Not 100% sure that's a reasonable approach so happy to consider a better alternative here if there is one.
|
We took a very different approach to wrapping the users |
Pass transfer-aware converters to Activity and Schedule invokers without rebuilding their configured options. Preserve serialization context for task-token heartbeats and propagate the configured converter to test Nexus clients.
Thanks for calling this out. I've taken a pass to consolidate the client side converter creation into I've left the worker side separate for now since the transfer type conversion happens in a workflow thread but the external storage must remain separate on that side. There may still be cleanup possible that I can explore if you think it could use some attention. |
Make the default converter transfer-aware while preserving its public API. Fix reflection-array decoding, callback exception handling, converter construction diagnostics, generic class declarations, and cache absence. Simplify client invoker and worker configuration. Strengthen direct and wrapped converter tests and correct Nexus operation-context coverage.
| this.options = | ||
| WorkflowClientOptions.newBuilder(options).setDataConverter(internalDataConverter).build(); |
There was a problem hiding this comment.
WE can move this above 108 so we have a builder already
| * @param valueType complete declared model type, including generic arguments | ||
| * @return non-null transfer type | ||
| */ | ||
| Type getTransferType(Type valueType); |
There was a problem hiding this comment.
Why do we need to take a Type as a parameter but Go's does not https://github.com/temporalio/sdk-go/blob/2dbb35e53c978457b461e0d8187ea9111f172b64/converter/transfer_types.go#L38
There was a problem hiding this comment.
Looking more it seems like we need this for generic resolution that will be needed for worker callbacks
| this.clientOptions = clientOptions; | ||
| this.namespace = clientOptions.getNamespace(); |
There was a problem hiding this comment.
Do we still need the clientOptions for anything else
| this.cache = cache; | ||
| ExternalStorageRunner externalStorageRunner = | ||
| ((WorkflowClientInternal) client.getInternal()).getExternalStorageRunner(); | ||
| WorkflowClientInternal clientInternal = (WorkflowClientInternal) client.getInternal(); |
There was a problem hiding this comment.
Probably can be reset?
This branch has not been deployed
2 participants
What changed?
@TransferTypeConvertibleandTransferTypeConverterAPIs for converting top-level model values to a transfer representation before data conversion.Why?
Breaking changes?
Server PR