Signal links#2889
Open
Evanthx wants to merge 19 commits into
Open
Conversation
…NexusOperationContext.java Co-authored-by: Alex Mazzeo <alex.mazzeo@outlook.com>
| handle.getInvoker().invoke(nexusRequest); | ||
| WorkflowExecution workflowExec = nexusStartWorkflowResponse.getWorkflowExecution(); | ||
|
|
||
| // If the start workflow response returned a link use it, otherwise |
Contributor
There was a problem hiding this comment.
Where did all this logic move?
| private final List<Link> responseBacklinks = new ArrayList<>(); | ||
| // Captured at construction (on the Nexus task executor's thread) and used to fail fast on any | ||
| // cross-thread mutation. See note on responseBacklinks. | ||
| private final Thread ownerThread; |
Contributor
There was a problem hiding this comment.
Don't think this is needed anymore?
| .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 |
Contributor
There was a problem hiding this comment.
Nit: We are stashing the links so that RPC's like signal can use them. More future proof IMO
| if (commonLink != null) { | ||
| inboundCommonLinks.add(commonLink); | ||
| } else { | ||
| log.warn( |
Contributor
There was a problem hiding this comment.
Nit: I would remove "signal propagation" here to account for more use cases
| } | ||
| startResponseBuilder.setFailure(dataConverter.exceptionToFailure(temporalFailure)); | ||
| } catch (Throwable failure) { | ||
| convertKnownFailures(failure); |
Contributor
There was a problem hiding this comment.
Why did we need to add this? Not saying it is wrong, just curious?
| } | ||
|
|
||
| /** | ||
| * Backlinks from every outbound RPC the handler issued. Never null; may be empty. Returned as an |
Contributor
There was a problem hiding this comment.
Suggested change
| * Backlinks from every outbound RPC the handler issued. Never null; may be empty. Returned as an | |
| * Backlinks from every outbound RPC the handler issued. Returned as an |
Annotate with @Nonnull
| public Link getStartWorkflowResponseLink() { | ||
| return startWorkflowResponseLink; | ||
| /** Links from the inbound Nexus task; empty if none. Never null. */ | ||
| public List<Link> getNexusOperationLinks() { |
Contributor
There was a problem hiding this comment.
| return startWorkflowResponseLink; | ||
| /** Links from the inbound Nexus task; empty if none. Never null. */ | ||
| public List<Link> getNexusOperationLinks() { | ||
| return nexusOperationLinks; |
Contributor
There was a problem hiding this comment.
Why is getBacklinks wrapped in unmodifiableList and this one not? Are they used differently?
| try { | ||
| OperationStartResult<HandlerResultContent> result = | ||
| startOperation(context, operationStartDetails.build(), input.build()); | ||
| // If signal or signalWithStart RPCs the handler issued returned backlinks, propagate them |
Contributor
There was a problem hiding this comment.
I think this comment is wrong, getBacklinks is also set by start workflow, I left this comment a few other places, but I would review all all the code comments and make sure they aren't being specific to signal since the code isn't now and will only get more general as we add more Nexus features.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sending signals through Nexus now populates a link from the sender to the receiver as well as one from the receiver back to the sender.
What was changed
Why?
Checklist
Closes
How was this tested: