-
Notifications
You must be signed in to change notification settings - Fork 214
Signal links #2889
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
base: master
Are you sure you want to change the base?
Signal links #2889
Changes from all commits
772725d
9ce8f8e
f15d07b
86fdfff
7daf427
91da0bb
c115d9c
d8ff825
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,9 @@ | |
| import io.temporal.common.interceptors.NexusOperationOutboundCallsInterceptor; | ||
| import io.temporal.nexus.NexusOperationContext; | ||
| import io.temporal.nexus.NexusOperationInfo; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| public class InternalNexusOperationContext { | ||
| private final String namespace; | ||
|
|
@@ -15,6 +18,21 @@ public class InternalNexusOperationContext { | |
| private final WorkflowClient client; | ||
| NexusOperationOutboundCallsInterceptor outboundCalls; | ||
| Link startWorkflowResponseLink; | ||
| // Links extracted from the inbound Nexus task. Stored once at the task-handler boundary so the | ||
| // workflow client (signal, signalWithStart) can attach them to outgoing requests via | ||
| // SignalWorkflowExecutionRequest.links, matching the Go SDK's NexusOperationLinksKey ctx value. | ||
| private List<Link> nexusOperationLinks = Collections.emptyList(); | ||
| // Backlinks returned by SignalWorkflowExecutionResponse.link / | ||
| // SignalWithStartWorkflowExecutionResponse.signal_link. One entry per signal RPC issued from | ||
| // within the Nexus operation handler. Drained by the task handler when building | ||
| // StartOperationResponse so every signal the handler issues gets a corresponding link on the | ||
| // caller workflow's history event. | ||
| // | ||
| // NOTE: this context is only safe for use from the single thread that runs the operation | ||
| // handler (the Nexus task executor's thread). Handlers that spawn their own threads to issue | ||
| // signals will not see the thread-local context, so the links from those signals will not | ||
| // propagate. | ||
| private final List<Link> signalWorkflowResponseLinks = new ArrayList<>(); | ||
|
|
||
| public InternalNexusOperationContext( | ||
| String namespace, | ||
|
|
@@ -68,6 +86,35 @@ public Link getStartWorkflowResponseLink() { | |
| return startWorkflowResponseLink; | ||
| } | ||
|
|
||
| /** | ||
| * Set the {@code common.v1.Link}s extracted from the inbound Nexus task so they can be attached | ||
| * to any signal RPCs issued by the operation handler. | ||
| */ | ||
|
Comment on lines
+89
to
+92
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is for signal RPCs currently, but will be other RPCs in the near-ish™️ future right? |
||
| public void setNexusOperationLinks(List<Link> links) { | ||
| this.nexusOperationLinks = links == null ? Collections.emptyList() : links; | ||
| } | ||
|
|
||
| /** Links from the inbound Nexus task; empty if none. Never null. */ | ||
| public List<Link> getNexusOperationLinks() { | ||
| return nexusOperationLinks; | ||
| } | ||
|
|
||
| /** | ||
| * Append a backlink returned by a signal-class RPC (signal or signalWithStart). Each signal the | ||
| * operation handler issues should add one entry; the task handler drains the list when building | ||
| * the operation's StartOperationResponse. | ||
| */ | ||
| public void addSignalWorkflowResponseLink(Link link) { | ||
| if (link != null) { | ||
| this.signalWorkflowResponseLinks.add(link); | ||
| } | ||
| } | ||
|
Comment on lines
+107
to
+111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if this is too operation specific. As more operations include links, we'll keep adding new getters/setters here and then have to ensure they're all consumed when building the StartOperationResponse. A perhaps contrived example would be a nexus operation that signals a workflow, updates a workflow, then starts a workflow would have backlinks for all of those if my understanding is correct. |
||
|
|
||
| /** Backlinks from every signal RPC issued by the handler. Never null; may be empty. */ | ||
| public List<Link> getSignalWorkflowResponseLinks() { | ||
| return signalWorkflowResponseLinks; | ||
| } | ||
|
|
||
| private class NexusOperationContextImpl implements NexusOperationContext { | ||
| @Override | ||
| public NexusOperationInfo getInfo() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| import io.temporal.failure.CanceledFailure; | ||
| import io.temporal.failure.TemporalFailure; | ||
| import io.temporal.internal.common.InternalUtils; | ||
| import io.temporal.internal.common.LinkConverter; | ||
| import io.temporal.internal.common.NexusUtil; | ||
| import io.temporal.internal.worker.NexusTask; | ||
| import io.temporal.internal.worker.NexusTaskHandler; | ||
|
|
@@ -284,6 +285,10 @@ private StartOperationResponse handleStartOperation( | |
| .setCallbackUrl(task.getCallback()) | ||
| .setRequestId(task.getRequestId()); | ||
| task.getCallbackHeaderMap().forEach(operationStartDetails::putCallbackHeader); | ||
| // Stash the inbound links in common.v1.Link form on the operation context so that signal | ||
| // RPCs issued by the handler (e.g. SignalWithStartWorkflow on the callee) can attach them | ||
| // to SignalWorkflowExecutionRequest.links. | ||
| List<io.temporal.api.common.v1.Link> inboundCommonLinks = new ArrayList<>(); | ||
| task.getLinksList() | ||
| .forEach( | ||
| link -> { | ||
|
|
@@ -296,7 +301,23 @@ private StartOperationResponse handleStartOperation( | |
| "Invalid link URL: " + link.getUrl(), | ||
| e); | ||
| } | ||
| // LinkConverter only returns a WorkflowEvent-shaped common.v1.Link; nexus links of | ||
| // other shapes (e.g. non-temporal URLs) come back null and are intentionally not | ||
| // forwarded onto SignalWorkflowExecutionRequest.links, which requires the | ||
| // WorkflowEvent variant. Log so a debugging session can see what was dropped. | ||
| io.temporal.api.common.v1.Link commonLink = | ||
| LinkConverter.nexusLinkToWorkflowEvent(link); | ||
| if (commonLink != null) { | ||
| inboundCommonLinks.add(commonLink); | ||
| } else { | ||
| log.warn( | ||
| "Dropping inbound Nexus link from outbound signal propagation: type='{}'," | ||
| + " url='{}' (not a parseable temporal WorkflowEvent link)", | ||
| link.getType(), | ||
| link.getUrl()); | ||
| } | ||
|
Comment on lines
+304
to
+318
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems incorrect to me. A SANO will have a link to a Nexus operation populated and it should be sent to signal (and other) RPCs. I think we'll need to include a more generic helper on the LinkConverter that converts a Nexus link based on the type field to the appropriate Temporal proto link type. |
||
| }); | ||
| CurrentNexusOperationContext.get().setNexusOperationLinks(inboundCommonLinks); | ||
|
|
||
| HandlerInputContent.Builder input = | ||
| HandlerInputContent.newBuilder().setDataStream(task.getPayload().toByteString().newInput()); | ||
|
|
@@ -307,10 +328,27 @@ private StartOperationResponse handleStartOperation( | |
| try { | ||
| OperationStartResult<HandlerResultContent> result = | ||
| startOperation(context, operationStartDetails.build(), input.build()); | ||
| // If signal/signalWithStart RPCs issued by the handler returned backlinks, propagate | ||
| // them to the caller so the caller workflow's history event links to each signal event | ||
| // on the callee. Same set of backlinks applies to both sync and async response variants. | ||
| List<io.temporal.api.nexus.v1.Link> signalBacklinks = new ArrayList<>(); | ||
| for (io.temporal.api.common.v1.Link signalResponseLink : | ||
| CurrentNexusOperationContext.get().getSignalWorkflowResponseLinks()) { | ||
| if (!signalResponseLink.hasWorkflowEvent()) { | ||
| continue; | ||
| } | ||
| io.temporal.api.nexus.v1.Link converted = | ||
| LinkConverter.workflowEventToNexusLink(signalResponseLink.getWorkflowEvent()); | ||
| if (converted != null) { | ||
| signalBacklinks.add(converted); | ||
| } | ||
| } | ||
|
Comment on lines
+331
to
+345
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to my consideration of RPC specific accessors above, we'd need to check every link accessor do processing here. I think it'd be cleaner here as well to avoid details of the specific RPC that the handler may have invoked. |
||
|
|
||
| if (result.isSync()) { | ||
| startResponseBuilder.setSyncSuccess( | ||
| StartOperationResponse.Sync.newBuilder() | ||
| .setPayload(Payload.parseFrom(result.getSyncResult().getDataBytes())) | ||
| .addAllLinks(signalBacklinks) | ||
| .build()); | ||
| } else { | ||
| startResponseBuilder.setAsyncSuccess( | ||
|
|
@@ -326,6 +364,7 @@ private StartOperationResponse handleStartOperation( | |
| .setUrl(link.getUri().toString()) | ||
| .build()) | ||
| .collect(Collectors.toList())) | ||
| .addAllLinks(signalBacklinks) | ||
| .build()); | ||
| } | ||
| } catch (OperationException e) { | ||
|
|
||
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.
IMO don't mention the Go SDK here