-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add first_execution_run_id to missing requests and responses #151
Conversation
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.
LGTM. Would rather not merge this until we get the server impl at least POC'd to confirm no more changes/quirks are needed. Rollbacks and backwards-incompatible changes in this repo are a bit rough.
We could probably add |
// If set, this call will error if the most recent (if no run id is set on | ||
// `workflow_execution`), or specified (if it is) workflow execution is not part of the same | ||
// execution chain as this id. |
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 found this comment hard to read. Better to break it into 2 sentences. For example:
If set, this call will error if the target workflow is not part of the same execution chain as this id. Target workflow is specified by workflow_execution
if run_id is set there, or the most recent run if run_id is not set on workflow_execution
.
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.
The PR only addresses a part of the "workflow chain overflow" scenario.
I think we need to hold off on merging it and instead address it holistically with the part of returning the "workflow chain IDs" back to the user.
See here for details: temporalio/temporal#2691
Yes, I agree @macrogreg. |
We're not doing this anytime soon. |
As discussed on slack, we need
first_execution_run_id
in all of these requests to make them safer.Also added this to
SignalWithStartWorkflowExecutionResponse
to make using stubs / handles created this way safer.