Add System Nexus transfer payload converter - #846
Conversation
cd1a3a2 to
32e2bca
Compare
tconley1428
changed the title
b98c798 to
9c62ddc
Compare
9c62ddc to
4ec540e
Compare
| using Xunit; | ||
| using Xunit.Abstractions; | ||
|
|
||
| public class SystemNexusTests : WorkflowEnvironmentTestBase |
There was a problem hiding this comment.
All of this looks to be just a move from WorkflowWorkerTests to SystemNexusTests, which is fine. But I don't see any net-new tests that validate the integration into ScheduleNexusOperationAsync method.
There was a problem hiding this comment.
Integration with it is currently not available since as you pointed out elsewhere, it doesn't use transfer type converter yet.
There was a problem hiding this comment.
I think we should add some kind of integration testing real soon. At least something that would have checked for the correct application of the converters to the different aspects of the ScheduleNexusOperationAsync invocation.
|
|
||
| // TODO(cretz): Support Nexus serialization context | ||
| var payloadConverter = instance.payloadConverterNoContext; | ||
| var payloadConverter = SystemNexusPayloadVisitor.IsSystemNexusEndpoint( |
There was a problem hiding this comment.
Have you considered the impact on the workflow replayer? I think similar support might be needed there.
There was a problem hiding this comment.
I haven't, here or in Python. That's a very good point.
There was a problem hiding this comment.
Added some coverage, but it appears to be the same path.
There was a problem hiding this comment.
I think we need to apply TemporalTransferTypePayloadConverter.Wrap at
TemporalClient at
There was a problem hiding this comment.
Yeah. Missed adding a transfer type convertible to the replay test. Added coverage for that in the next commit.
dcd6df6 to
8af354c
Compare
8af354c to
0cff561
Compare
| @@ -2761,7 +2765,7 @@ public override Task<NexusWorkflowOperationHandle<TResult>> ScheduleNexusOperati | |||
| // If there is a start sync fail, we have to fail the handle task and | |||
| // there's nothing more we can do here | |||
| var handle = new NexusWorkflowOperationHandleImpl<TResult>( | |||
There was a problem hiding this comment.
This needs to be passed the instance.payloadConverterNoContext so it can be used at https://github.com/temporalio/sdk-dotnet/blob/dcf7ca3f4fc230315c6f518e3f3c1a1f52670963/src/Temporalio/Worker/WorkflowInstance.cs#L3187C53-L3187C60
There was a problem hiding this comment.
I don't think these are correct, but I might want to walk through your logic with you to be sure.
| handleSource.SetException( | ||
| instance.failureConverterNoContext.ToException( | ||
| syncStartFail, payloadConverter)); | ||
| syncStartFail, operationPayloadConverter)); |
There was a problem hiding this comment.
This needs to use instance.payloadConverterNoContext.
|
|
||
| // TODO(cretz): Support Nexus serialization context | ||
| var payloadConverter = instance.payloadConverterNoContext; | ||
| var payloadConverter = SystemNexusPayloadVisitor.IsSystemNexusEndpoint( |
There was a problem hiding this comment.
I think we need to apply TemporalTransferTypePayloadConverter.Wrap at
TemporalClient at
| using Xunit; | ||
| using Xunit.Abstractions; | ||
|
|
||
| public class SystemNexusTests : WorkflowEnvironmentTestBase |
There was a problem hiding this comment.
I think we should add some kind of integration testing real soon. At least something that would have checked for the correct application of the converters to the different aspects of the ScheduleNexusOperationAsync invocation.
2 participants
Summary
Generated System Nexus support will own the nested converter context in a follow-up once the
nex-gen transfer-type changes are ingested.
This mirrors the Python System Nexus converter: it owns transfer conversion for the outer envelope
and scopes the user converters while generated System Nexus transfer types serialize nested values.
Validation
dotnet build tests/Temporalio.Tests/Temporalio.Tests.csproj --no-restoredotnet test tests/Temporalio.Tests/Temporalio.Tests.csproj --no-build --filter FullyQualifiedName~SystemNexusPayloadConverterTests --verbosity normaldotnet test tests/Temporalio.Tests/Temporalio.Tests.csproj --no-build --filter FullyQualifiedName~SystemNexusTests --verbosity minimal