[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.
| 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.
| 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.
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.
| } | ||
| 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 | |
| }, |
| // 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.
What changed?
Why?
How did you test it?
Potential risks
Documentation
Is hotfix candidate?