-
Notifications
You must be signed in to change notification settings - Fork 97
Wire request_id through CallbackInfo for linking #851
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
Changes from 1 commit
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 |
|---|---|---|
|
|
@@ -68,8 +68,7 @@ message WorkflowExecution { | |
| string run_id = 2; | ||
| } | ||
|
|
||
| // Identifies a specific execution within a namespace. This is used for standalone activities | ||
| // executions in batch jobs currently. | ||
| // Identifies a specific execution within a namespace. | ||
| message Execution { | ||
| temporal.api.enums.v1.ExecutionType type = 1; | ||
| string business_id = 2; | ||
|
|
@@ -304,13 +303,14 @@ message Link { | |
| string reason = 4; | ||
| } | ||
|
|
||
| // A link to a worker callback attached to a Nexus operation within the same namespace. e.g. the completion handler attached | ||
| // to a standalone Nexus operation, not the source Nexus operation itself. A standalone Nexus operation can have multiple | ||
| // callbacks attached to them, and will be differentiated by the server-generated request_id used when the callback was invoked. | ||
| message NexusOperationCallback { | ||
| string operation_id = 1; | ||
| string run_id = 2; | ||
| string request_id = 3; | ||
| // A link to a worker callback attached to an execution. An execution (e.g. workflow or standalone Nexus | ||
| // operation) can have multiple callbacks attached to them, and will be differentiated by the request_id used when the callback | ||
| // is invoked. | ||
| message Callback { | ||
| // Source execution the callback was attached to. | ||
| Execution execution = 1; | ||
| // Request ID used for the callback's delivery. | ||
| string request_id = 2; | ||
|
Contributor
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 think we should consider creating a "callback ID" as fully supported thing. The ambiguity where the request ID can be shared across multiple callbacks feels a little awkward.
Author
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. Strong +1 to this. We just need a way to uniquely identify a worker callback within the scope of an execution, and having an entirely server-side generated ID removes any potential confusion. Moreover, having it be an entirely Temporal-managed
Member
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. There's no user-editable callback::id. I'd be more than okay using the request ID as the callback ID since it is a unique identifier as long as we document it. |
||
| } | ||
|
|
||
| oneof variant { | ||
|
|
@@ -319,7 +319,7 @@ message Link { | |
| Activity activity = 3; | ||
| NexusOperation nexus_operation = 4; | ||
| Workflow workflow = 5; | ||
| NexusOperationCallback nexus_operation_callback = 6; | ||
| Callback callback = 6; | ||
| } | ||
| } | ||
|
|
||
|
|
||
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 would decouple the user provided request ID from the one the system generates. The start request is not the same as the callback delivery request and those should have different IDs. I would be confused if I saw the same request ID provided in a log for different purposes.
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 concerned that this will be confused with the request ID of the start request used to attach this callback. Document the semantics as we did here:
api/temporal/api/nexus/v1/message.proto
Lines 308 to 311 in 7868510
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.
Thanks for linking to the comment, I agree that's exactly how we'd want to define it.