Add links and callbacks support for standalone activity#759
Conversation
fretz12
changed the title
dandavison
left a comment
dandavison
left a comment
There was a problem hiding this comment.
Looks great but I think that we need to make CallbackInfo handle Activity before merging.
| bytes long_poll_token = 5; | ||
|
|
||
| // Callbacks attached to this activity execution and their current state. | ||
| repeated temporal.api.workflow.v1.CallbackInfo callbacks = 6; |
There was a problem hiding this comment.
This CallbackInfo is currently workflow-specific:
// CallbackInfo contains the state of an attached workflow callback.
message CallbackInfo {
// Trigger for when the workflow is closed.
message WorkflowClosed {}
message Trigger {
oneof variant {
WorkflowClosed workflow_closed = 1;
}
}We either need to make it more general, or add an activity-specific CallbackInfo. I'm thinking we'd want the former -- add an ActivityClosed or ExecutionClosed trigger. We're going to want to straighten this out, and it would be a breaking API change to do it later.
There was a problem hiding this comment.
Ah I totally missed this, this callback info struct is only for workflows. You'll need to create a callback info struct for activities instead.
There was a problem hiding this comment.
Good call, added an activity specific cb
| repeated temporal.api.common.v1.Callback completion_callbacks = 19; | ||
| // Links to be associated with the activity. Callbacks may also have associated links; | ||
| // links already included with a callback should not be duplicated here. | ||
| repeated temporal.api.common.v1.Link links = 20; |
There was a problem hiding this comment.
The SAA PR only uses links on the callback. Is it necessary/appropriate to add these top-level links in addition to the callback links? If so, could the Nexus team give a refresher on what the respective roles are of the two links (I had it in my head that top-level were deprecated).
There was a problem hiding this comment.
Not deprecated, I commented on this before. It's possible to link without attaching callbacks.
| // Set if activity cancelation was requested. | ||
| string canceled_reason = 32; | ||
|
|
||
| // Links associated with the activity. |
There was a problem hiding this comment.
A reader is going to think initially these are links that in some way refer to this activity. However, they're not -- they're links pointing to other entities associated with the activity; typically the entity that started the activity. Let's clarify this for the reader, here and in the workflow counterpart HistoryEvent.links. As an author of a Nexus SDK I found this confusing at first, so there's not much doubt that other users will benefit from some help here.
| // Links associated with the activity. | |
| // Links to entities associated with this activity, for example | |
| // to the entity that started this activity. |
There was a problem hiding this comment.
Gotcha, changed here and at HistoryEvent
Added links/callbacks to describe and start responses Update openapi
Co-authored-by: Quinn Klassen <klassenq@gmail.com>
Co-authored-by: Spencer Judge <spencer@temporal.io>
fretz12
force-pushed
the
fredtzeng/saa-api-features
branch
from
8f2ca2c to
2bc9df1
Compare
|
|
||
| // If the state is BLOCKED, blocked reason provides additional information. | ||
| string blocked_reason = 9; | ||
| } |
There was a problem hiding this comment.
I think this is the right design; just describing it to make it explicit. So each CHASM component will have its own CallbackInfo, the role of which is to describe a callback's state and triggering condition. Since different CHASM components will have their own component-specific sets of possible triggering conditions, we define component-specific CallbackInfo types. In practice the only field that differs for now is that trigger oneof; they'll share identical callback state fields.
There was a problem hiding this comment.
Agree, we should ideally have a single type for callback info since it will be used across multiple archetypes. The trigger is the only archetype specific information.
I'm going to walk back what I wrote yesterday and ask you to create a temporal.api.callback.v1.CallbackInfo which has all of the common fields and put only Trigger and temporal.api.callback.v1.CallbackInfo in this message.
There was a problem hiding this comment.
I refactored the common fields to temporal.api.callback.v1.CallbackInfo. Left workflow callback info as is to avoid breaking changes
| // Trigger for this callback. | ||
| Trigger trigger = 1; | ||
| // Common callback info. | ||
| temporal.api.callback.v1.CallbackInfo callback_info = 2; |
There was a problem hiding this comment.
We're thinking let's call this info.
5 participants
What changed?
Why?
To support links and callbacks for standalone activity executions (SAA), bringing feature parity with workflow-based activity scheduling.