-
Notifications
You must be signed in to change notification settings - Fork 48
Nexus support #517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Nexus support #517
Conversation
# Conflicts: # src/Temporalio/Bridge/Interop/Interop.cs # src/Temporalio/Bridge/OptionsExtensions.cs # src/Temporalio/Bridge/include/temporal-sdk-bridge.h # src/Temporalio/Bridge/src/worker.rs # src/Temporalio/Temporalio.csproj # src/Temporalio/Worker/TemporalWorker.cs
src/Temporalio/Temporalio.csproj
Outdated
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="2.2.0" /> | ||
| <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.1.1" PrivateAssets="All" /> | ||
| <PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0" PrivateAssets="All" /> | ||
| <ProjectReference Include="..\..\..\nexus-sdk-dotnet\src\NexusRpc\NexusRpc.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't pass CI until nexus-rpc/sdk-dotnet#4 merged/published
| /// <param name="Namespace">Current namespace.</param> | ||
| /// <param name="TaskQueue">Current task queue.</param> | ||
| /// <remarks>WARNING: Nexus support is experimental.</remarks> | ||
| public record NexusOperationInfo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this exposed to users? I am not sure we exposed anything similar in Go or Java? Not saying we shouldn't just if we do we should open issue in Go and Java for parity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This info was exposed in Python by @dandavison. We can make this one and that one internal only or we can open Go/Java/TypeScript issues. Do you have a preference/vote?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with adding it, might be worth having a way for a user to detect they are in a Nexus operation context like we have for activities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, setting reminder to open issues in Go, Java, and TypeScript to ensure it is included there too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have opened temporalio/features#674 to track it in general
| OperationName: operationName, | ||
| Arg: arg, | ||
| Options: options ?? new(), | ||
| Headers: null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you pass null headers vs an empty map? If that is what we did in other places in the dotnet SDK not suggesting we change, just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is what we did in other places. .NET devs are self-conscious about unnessesarily creating new objects if they don't have to and they aren't used often. For headers, to create an empty mutable dictionary would be extra unnecessary object creation in most cases (and immutable dictionary w/ copy-on-write semantics is not a common .NET approach here). Granted .NET has good short-lived-object GC like Java these days so it's not a big deal, but it's more proper to allow nullable here (which can be expressed via the type system so users are not surprised).
| appExc, | ||
| HandlerErrorRetryBehavior.NonRetryable); | ||
| } | ||
| else if (exc is WorkflowAlreadyStartedException) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any test for this behaviour? Would be good to have some tests for the general RPC exception to handler translation, but IMO this is the more important error to translate since most async nexus operations are just starting workflows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 No, will add test. The tests I had done so far on dupe checking were more about checking conflict policy. I will add a test that confirms regular start will fail as non-retryable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, just realized we do have a test that confirms this, it is part of the ExecuteNexusOperationAsync_SimpleWorkflow_ConflictPolicy test (the test confirms already-started behavior before it confirms conflict-policy-use-existing behavior)
Quinn-With-Two-Ns
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other then my previous comments LGTM
# Conflicts: # src/Temporalio/Bridge/Interop/Interop.cs
| Endpoint = input.ClientOptions.Endpoint, | ||
| Service = input.Service, | ||
| Operation = input.OperationName, | ||
| Input = input.Arg == null ? null : instance.PayloadConverter.ToPayload(input.Arg), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to confirm this PayloadConverter does not have the workflow context attached correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does as do all workflow calls. Based on off-PR discussions ,I have opened a separate issue at #523 to solve this generally (it is not a Nexus-specific concern).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should fix this for Nexus to not make the problem worse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't just fix in Nexus, we have to fix everywhere. It is a general fix, not a Nexus specific fix. I do not think we should hold up experimental Nexus support for a general fix to this problem.
| var token = input.Options.CancellationToken ?? instance.CancellationToken; | ||
| // We do not even want to schedule if the cancellation token is already cancelled. | ||
| // We choose to use cancelled failure instead of wrapping in Nexus failure which is | ||
| // similar to what Java and TypeScript do, with the accepted tradeoff that it makes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // similar to what Java and TypeScript do, with the accepted tradeoff that it makes | |
| // similar to what TypeScript do, with the accepted tradeoff that it makes |
Java does wrap the cancelled failure in a Nexus failure. What does dotnet do in other immediate cancellation scenarios ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, this is copied from the child workflow comment. I was under the impression for child/nexus cancel-before-start, that Java and TypeScript just throw (i.e. set promise result to) a CanceledFailure, not a wrapped cancel failure as would happen if the cancel happened after schedule/start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression for child/nexus cancel-before-start, that Java and TypeScript just throw (i.e. set promise result to) a CanceledFailure
That is not true for Java, cancel-before-start throws the same error chain as cancel happened after schedule/start.
What was changed
Added Nexus support. Notes:
Checklist