[Will NOT Merge] Chasm interface draft #6987
Conversation
yycptt
requested review from
bergundy,
Sushisource,
alexshtin,
lina-temporal and
paulnpdev
There was a problem hiding this comment.
I would put this in a top level chasm directory. There's likely going to be some chasm related code in other services.
| // | ||
| // Framework will try to recognize the type and do serialization/deserialization | ||
| // proto.Message is recommended so the component get compatibility if state definition changes | ||
| State persistencepb.ActivityInfo // proto.Message |
There was a problem hiding this comment.
Let's call this standard field Data, since State is usually an enum.
There was a problem hiding this comment.
Alternatively, we can let you embed a proto and treat that as the data field. That could be slightly nicer to use.
There was a problem hiding this comment.
Agree with naming change. Disagree with proto embed. I'd prefer for it to just be explicit.
| Input *chasm.Field[*common.Payload] `chasm:"lazy"` | ||
| Output *chasm.Field[*common.Payload] `chasm:"lazy"` | ||
|
|
||
| EventNotifier *chasm.Field[EventNotifier] |
There was a problem hiding this comment.
I know this is just to show the pointer capabilities, but the real activity implementation will need to do more than just notify its parent, it'll need to be injected with a way to read the inputs and outputs. They won't be embedded payloads in the workflow case.
There was a problem hiding this comment.
I also had thought we wanted a way for the parent to be able to decide what notifications it cared about from its children, rather than the children needing to remember to notify the parent. I think that can still be implemented in terms of this a pointer just fine though.
Something as simple as the following I think would be nice, and isn't really even part of the framework, just a useful pattern. I think it keeps things a little more obvious for the activity implementer.
func NewScheduledActivity(
chasmContext chasm.MutableContext,
params *NewActivityRequest,
onComplete func(ActivityCompletedEvent)
) { ... }
// Then workflow constructs like
NewScheduledActivity(ctx, params, func(ce) { me.AsComponentPointer(ctx).OnCompletion(ce) })
| output := &common.Payload{ | ||
| Data: req.Output, | ||
| } | ||
| i.Output = chasm.NewData(chasmContext, output) |
There was a problem hiding this comment.
I'd probably call this method NewDataField since it constructs a Field.
|
|
||
| func (l Library) Components() []chasm.RegistrableComponent { | ||
| return []chasm.RegistrableComponent{ | ||
| chasm.NewRegistrableComponent[*Activity]( |
There was a problem hiding this comment.
I'm kinda going back and forth between a RegisterComponent function and this approach where the library returns the list of components.
The main downside to this approach is that you have to wrap with NewRegisterableComponent.
If we do want this approach, we may want a LibraryBase struct that has empty implementations for all of the methods so library implementors only provide the items they need registered.
I like that with the concept of a library, you essentially get a namespace for all components and tasks.
There was a problem hiding this comment.
I am in favor of returning the list of components. Having to wrap seems like a total non-issue for something that's done this infrequently.
There was a problem hiding this comment.
This is a library, and should be put in chasm/lib/activity/ IMHO.
There was a problem hiding this comment.
Library specific RPCs, configs, and protos should also be colocated in the lib directory.
| "go.temporal.io/server/service/history/chasm" | ||
| ) | ||
|
|
||
| // This will be nexus |
There was a problem hiding this comment.
👍 I'll work on a Nexus POC on top of this.
|
|
||
| type TimeoutTaskHandler struct{} | ||
|
|
||
| func (h *TimeoutTaskHandler) Validate( |
There was a problem hiding this comment.
A little on the fence if Validate should be here or on the Component or the task but this works and has some advantages.
|
|
||
| func (h *TimeoutTaskHandler) Validate( | ||
| chasmContext chasm.Context, | ||
| activity *Activity, |
There was a problem hiding this comment.
I wonder if we reuse tasks for separate components and there will need to be a way to provide a Validate function for each of them.
| return nil, err | ||
| } | ||
|
|
||
| resp, startedActivityRef, err := chasm.UpdateComponent( |
There was a problem hiding this comment.
As discussed yesterday, I wish we could get the ref in the update function and keep the ref out of this signature. This is good to start with though.
There was a problem hiding this comment.
Yeah, ideally a component author making a transition doesn't need to care where the referenced component came from
| } | ||
| if err := chasmContext.AddTask( | ||
| i, | ||
| chasm.TaskAttributes{ |
There was a problem hiding this comment.
I like that you've separated this out and not put it in the Task like I had it in my POC.
| func(a *Activity, ctx chasm.Context, _ *GetActivityResultRequest) bool { | ||
| return a.LifecycleState() == chasm.LifecycleStateCompleted | ||
| }, | ||
| func(a *Activity, ctx chasm.MutableContext, _ *GetActivityResultRequest) (*GetActivityResultResponse, error) { | ||
| outputPayload, err := a.Output.Get(ctx) | ||
| resp.Output = outputPayload.Data | ||
| return resp, err | ||
| }, |
There was a problem hiding this comment.
I wonder if we'd be better off merging these two like so:
| func(a *Activity, ctx chasm.Context, _ *GetActivityResultRequest) bool { | |
| return a.LifecycleState() == chasm.LifecycleStateCompleted | |
| }, | |
| func(a *Activity, ctx chasm.MutableContext, _ *GetActivityResultRequest) (*GetActivityResultResponse, error) { | |
| outputPayload, err := a.Output.Get(ctx) | |
| resp.Output = outputPayload.Data | |
| return resp, err | |
| }, | |
| func(a *Activity, ctx chasm.MutableContext, _ *GetActivityResultRequest) (*GetActivityResultResponse, error) { | |
| if a.LifecycleState() != chasm.LifecycleStateCompleted { | |
| return nil, chasm.ErrNoReady / *name TBD */ | |
| } | |
| outputPayload, err := a.Output.Get(ctx) | |
| resp.Output = outputPayload.Data | |
| return resp, err | |
| }, |
There was a problem hiding this comment.
I think I prefer them separate, because what happens if you mutate something and then say "not ready"? That would be some weird violation that shouldn't be possible, and separate contexts enforces that at the type level.
If you need context from one to the other, make the ready function return (bool, T) and T is passed into the mutator
| // panic("not implemented") | ||
| // } | ||
|
|
||
| func NewEntity[C Component, I any, O any]( |
There was a problem hiding this comment.
Alternatively, call this CreateEntity but what you have is fine too.
| panic("not implemented") | ||
| } | ||
|
|
||
| // Not needed for V1 |
There was a problem hiding this comment.
True, this isn't needed but if we were to do it, there's a type safe way using a closure that gets an instance and returns field references.
| // | ||
| // Framework will try to recognize the type and do serialization/deserialization | ||
| // proto.Message is recommended so the component get compatibility if state definition changes | ||
| State persistencepb.ActivityInfo // proto.Message |
There was a problem hiding this comment.
Agree with naming change. Disagree with proto embed. I'd prefer for it to just be explicit.
| } | ||
|
|
||
| func (i *Activity) GetDispatchInfo( | ||
| chasmContext chasm.MutableContext, |
There was a problem hiding this comment.
Should this be an immutable context?
| output := &common.Payload{ | ||
| Data: req.Output, | ||
| } | ||
| i.Output = chasm.NewData(chasmContext, output) |
|
|
||
| func (l Library) Components() []chasm.RegistrableComponent { | ||
| return []chasm.RegistrableComponent{ | ||
| chasm.NewRegistrableComponent[*Activity]( |
There was a problem hiding this comment.
I am in favor of returning the list of components. Having to wrap seems like a total non-issue for something that's done this infrequently.
| Input *chasm.Field[*common.Payload] `chasm:"lazy"` | ||
| Output *chasm.Field[*common.Payload] `chasm:"lazy"` | ||
|
|
||
| EventNotifier *chasm.Field[EventNotifier] |
There was a problem hiding this comment.
I also had thought we wanted a way for the parent to be able to decide what notifications it cared about from its children, rather than the children needing to remember to notify the parent. I think that can still be implemented in terms of this a pointer just fine though.
Something as simple as the following I think would be nice, and isn't really even part of the framework, just a useful pattern. I think it keeps things a little more obvious for the activity implementer.
func NewScheduledActivity(
chasmContext chasm.MutableContext,
params *NewActivityRequest,
onComplete func(ActivityCompletedEvent)
) { ... }
// Then workflow constructs like
NewScheduledActivity(ctx, params, func(ce) { me.AsComponentPointer(ctx).OnCompletion(ce) })| return nil, err | ||
| } | ||
|
|
||
| resp, startedActivityRef, err := chasm.UpdateComponent( |
There was a problem hiding this comment.
Yeah, ideally a component author making a transition doesn't need to care where the referenced component came from
| func(a *Activity, ctx chasm.Context, _ *GetActivityResultRequest) bool { | ||
| return a.LifecycleState() == chasm.LifecycleStateCompleted | ||
| }, | ||
| func(a *Activity, ctx chasm.MutableContext, _ *GetActivityResultRequest) (*GetActivityResultResponse, error) { | ||
| outputPayload, err := a.Output.Get(ctx) | ||
| resp.Output = outputPayload.Data | ||
| return resp, err | ||
| }, |
There was a problem hiding this comment.
I think I prefer them separate, because what happens if you mutate something and then say "not ready"? That would be some weird violation that shouldn't be possible, and separate contexts enforces that at the type level.
If you need context from one to the other, make the ready function return (bool, T) and T is passed into the mutator
| ) | ||
|
|
||
| type Context interface { | ||
| // Context is not binded to any component, |
There was a problem hiding this comment.
| // Context is not binded to any component, | |
| // Context is not bound to any component, |
| // we probably don't even need this, | ||
| // can make the function generic and find the name from registry | ||
| rootComponentName: rootComponentName, |
There was a problem hiding this comment.
Agree. Taking in the type and having components define their name so you operate in terms of types rather than names is nice.
| // If we provide this method, then the method on the engine doesn't need to | ||
| // return a Ref | ||
| // NewRef(Component) (ComponentRef, bool) |
There was a problem hiding this comment.
You could have a CreationContext that simply doesn't include this, and that and MutableContext both embed an un-exported base interface.
What changed?
Why?
How did you test it?
Potential risks
Documentation
Is hotfix candidate?