Nexus: use links from StartWorkflowExecutionResponse if present #963
Conversation
| @@ -1550,6 +1556,7 @@ def __init__( | |||
| self._result_run_id = result_run_id | |||
| self._first_execution_run_id = first_execution_run_id | |||
| self._result_type = result_type | |||
| self._start_workflow_response = start_workflow_response | |||
| self.__temporal_eagerly_started = False | |||
There was a problem hiding this comment.
We need to make the links from the StartWorkflow response available to code that is in possession of a WorkflowHandle. This PR is currently proposing to attach the entire reponse proto to the workflow handle, but only exposed via an underscore-prefixed attribute, so that it can be used by SDK code but it is not advertised as a stable public API for now.
There was a problem hiding this comment.
Mentioned off-PR, but will add here too...
This has been an issue in other SDKs. I have proposed that the start caller provide something that can be populated with extra things instead of tacking it on the handle which has more use than just as a start result holder. For instance, have start_workflow accept a mutable StartWorkflowResponseDetails that can have these extra things set on it from the response.
In addition to this, there are actually three valuable user-facing values that can come back from start we should expose: 1) started - whether it actually ended up starting a workflow, which can be false in cases of use-existing conflict policy, 2) status - there is a rare case with update with start where a start workflow succeeds without resulting in a running workflow, and 3) eagerly_started - whether it eagerly started which users may want to know.
We need this across SDKs, it's not a Python only need, and I think we can take the same approach.
| @@ -146,25 +147,30 @@ def _get_workflow_event_links( | |||
| def _add_outbound_links( | |||
| self, workflow_handle: temporalio.client.WorkflowHandle[Any, Any] | |||
| ): | |||
| # If links were not sent in StartWorkflowExecutionResponse then construct them. | |||
There was a problem hiding this comment.
Is it valuable to even parse the link? Can we just take any link w/ workflow event on it from the response and set it on outbound?
There was a problem hiding this comment.
No because outbound is list[nexusrpc.Link]
There was a problem hiding this comment.
I see. So I wonder then what the server links has that makes it better than the one manually built here, but I can understand if the server one should be the canonical one.
dandavison
force-pushed
the
nexus-use-links-from-start-workflow-response
branch
from
62c2533 to
857f864
Compare
| @@ -146,25 +147,30 @@ def _get_workflow_event_links( | |||
| def _add_outbound_links( | |||
| self, workflow_handle: temporalio.client.WorkflowHandle[Any, Any] | |||
| ): | |||
| # If links were not sent in StartWorkflowExecutionResponse then construct them. | |||
There was a problem hiding this comment.
I see. So I wonder then what the server links has that makes it better than the one manually built here, but I can understand if the server one should be the canonical one.
dandavison
force-pushed
the
nexus-use-links-from-start-workflow-response
branch
from
857f864 to
c4ffc2c
Compare
dandavison
force-pushed
the
nexus-use-links-from-start-workflow-response
branch
from
c4ffc2c to
62ea494
Compare
On starting a nexus-backed workflow, a Nexus handler implementation must return to the nexus caller a link to the started workflow.
Newer server versions return the link to the started workflow in the
StartWorkflowresponse. Nexus implementations, on starting a nexus-backed workflow, should use the link in the server's response if present, falling back to constructing the link themselves.temporalio/sdk-go#1934 is an analogous PR.