Add Event Groups #793
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package temporal.api.sdk.v1; | ||
|
|
||
| option go_package = "go.temporal.io/api/sdk/v1;sdk"; | ||
| option java_package = "io.temporal.api.sdk.v1"; | ||
| option java_multiple_files = true; | ||
| option java_outer_classname = "EventGroupMarkerProto"; | ||
| option ruby_package = "Temporalio::Api::Sdk::V1"; | ||
| option csharp_namespace = "Temporalio.Api.Sdk.V1"; | ||
|
|
||
|
|
||
| import "temporal/api/common/v1/message.proto"; | ||
|
|
||
| message EventGroupMarker { | ||
| // Opaque identifier assigned by the SDK. | ||
| // | ||
| // Construction of this ID follows strict rules to ensure history | ||
| // consolidation is possible. See below. | ||
| string id = 1; | ||
|
|
||
| // Attributes attached to the group marker. They only needs to be set once per | ||
| // marker ID; further references to an existing marker ID reuse existing | ||
| // attributes of the referenced marker. | ||
| // | ||
| // In the case where a later group marker does carry attributes, then those | ||
| // attributes override the previous attributes of the same marker ID for all | ||
| // events carrying that marker ID, including preceding events. | ||
| oneof attributes { | ||
| LabelAttributes label_attributes = 2; | ||
| InboundEventAttributes inbound_event_attributes = 3; | ||
| InboundUpdateAttributes inbound_update_attributes = 4; | ||
|
Comment on lines
+30
to
+32
Member
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 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?
Contributor
Author
There was a problem hiding this comment.
Well, that would be technically feasible, though definitely not what I had in mind.
An event can be attached to multiple groups, so But at that point, I'd simply suggest the user to explicitly create a labelled group wrapping the complete signal/update handler's code.
Member
There was a problem hiding this comment.
Yeah, reasonable, and you're saying those commands would be part of both the auto and manual group. Makes sense. |
||
| } | ||
|
|
||
| // A user-defined short-form string value to be used as the group's label. | ||
| message LabelAttributes { | ||
| // This payload should be a "json/plain"-encoded payload that is a single | ||
| // JSON string for use in user interfaces. User interface formatting may not | ||
| // apply to this text when used in "label" situations. The payload data | ||
| // section is limited to 400 bytes by default. | ||
| // | ||
| // 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; | ||
|
Member
There was a problem hiding this comment. This is
Contributor
Author
There was a problem hiding this comment. Indeed. Basically, this follows the convention we used for |
||
| } | ||
|
|
||
| // The event ID of an event in the present workflow that triggered implicit | ||
| // creation of this group marker. The target event's type must be one of the | ||
| // following: | ||
| // - `WORKFLOW_EXECUTION_STARTED` | ||
| // - `WORKFLOW_EXECUTION_SIGNALED` | ||
| message InboundEventAttributes { | ||
| int64 inbound_event_id = 1; | ||
| } | ||
|
|
||
| // The workflow-unique identifier of an inbound Update whose handler triggered | ||
| // implicit creation of this group marker. | ||
| // | ||
| // Used in place of `inbound_event_id` for Updates because the event ID of the | ||
| // UpdateAccepted history event is not known until the Workflow Task is | ||
| // completed and recorded by the server, which may be too late. | ||
| message InboundUpdateAttributes { | ||
| string inbound_update_id = 1; | ||
| } | ||
| } | ||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
UserMetadatawould 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
UserMetadataon 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.Uh oh!
There was an error while loading. Please reload this page.
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.
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.