Add Event Groups#793
Conversation
| temporal.api.sdk.v1.UserMetadata user_metadata = 301; | ||
|
|
||
| // Event Group Markers attached to the command by the workflow author. | ||
| repeated temporal.api.sdk.v1.EventGroupMarker event_group_markers = 302; |
There was a problem hiding this comment.
Why here rather than in UserMetadata? Not necessarily opposed, just w/ UserMetadata I think we probably can avoid touching server at all.
There was a problem hiding this comment.
Short story is that really depends on where we want to go with Event Groups. What we have in the MVP is really just an extension of the existing User Metadata concept, only providing minor enrichment to the workflow history. If we stop there, adding this to UserMetadata would indeed be a possibility (that was actually my initial direction).
However, envisioned goals for the proposal include some features that could benefit from Event Groups being their own thing rather than piggybacked on User Metadata. For example, one of the near-term goal is to allow requesting cancellation of an Event Group. Another one is to be able to follow Event Group across workflows. In both cases, there are some implicit requirements regarding the validity of the data stored in event groups, stronger than what we currently have on user metadata.
Even given the immediate MVP scope, the fact is that we accept UserMetadata on many APIs that can't accept Event Groups. So we'd need to spread server-side checks everywhere to confirm that supplied user metadata doesn't contain event groups. That's way more painful than adding this field here and add explicit copy assignments to the server in the very few places (three to be exact) where that's pertinent.
There was a problem hiding this comment.
the fact is that we accept UserMetadata on many APIs that can't accept Event Groups.
Yeah, that makes sense. The previous points I feel like we still have the data well formed even if it's in UserMetadata, but this makes a lot of sense to me.
| LabelAttributes label_attributes = 2; | ||
| InboundEventAttributes inbound_event_attributes = 3; | ||
| InboundUpdateAttributes inbound_update_attributes = 4; |
There was a problem hiding this comment.
I get event/update being mutually exclusive, but, I think it'd be nice to be able to have explicit labels at the same time as it being auto-triggered? We could have some kind of getCurrentGroup API that you could then attach labels to?
Or, does the comment imply you could have two of these markers, one label, and one inbound, for the same ID and that enables this?
There was a problem hiding this comment.
Does the comment imply you could have two of these markers, one label, and one inbound, for the same ID and that enables this?
Well, that would be technically feasible, though definitely not what I had in mind.
I think it'd be nice to be able to have explicit labels at the same time as it being auto-triggered? We could have some kind of getCurrentGroup API that you could then attach labels to?
An event can be attached to multiple groups, so getCurrentGroup() is not exactly possible. We could have getCurrentGroups(), but then that would be painful for users to do what you describe. Or we could have some form of getCurrentSignalUpdateOrWorkflowEventGroup() (just need to figure out a name), for which there is always one and only one at any point...
But at that point, I'd simply suggest the user to explicitly create a labelled group wrapping the complete signal/update handler's code.
There was a problem hiding this comment.
I'd simply suggest the user to explicitly create a labelled group wrapping the complete signal/update handler's code.
Yeah, reasonable, and you're saying those commands would be part of both the auto and manual group. Makes sense.
| // | ||
| // Note that it is valid to have distinct markers (i.e. distinct marker IDs) | ||
| // in a given workflow execution that carry the same label. | ||
| temporal.api.common.v1.Payload label = 1; |
There was a problem hiding this comment.
This is Payload just to allow for encryption I assume?
There was a problem hiding this comment.
Indeed. Basically, this follows the convention we used for summary and details.
2 participants
What changed?
temporal.api.sdk.v1.EventGroupMarkermessage.event_group_markersonCommandandHistoryEvent.Why?
Event Groups is a new form of metadata that allows regrouping of logically related Workflow Events, according to user-defined or system-inferred criteria, providing users with improved visibility, analysis, and debugging capabilities.
Breaking changes
No.
Server PR
temporalio/temporal#10472