Conversation
| Discard(context.Context, ComponentRef, TaskAttributes, T) error | ||
|
|
||
| // TaskGroup returns an identifier used to group side-effect tasks together for outbound circuit breaking | ||
| TaskGroup() string |
There was a problem hiding this comment.
hmm I think it's a property of the Task itself not the TaskHandler.
I think we have RegistrableTaskOption type defined for this purpose, can we use that instead and it will be the first use case. :)
There was a problem hiding this comment.
I think putting it on the task handler is more flexible since you can act on a concrete task instance and dynamically set the group. But I don't have a concrete use case in mind so I'm okay with what @yycptt is suggesting.
|
|
||
| if ct, ok := task.(*tasks.ChasmTask); ok && m.chasmRegistry != nil { | ||
| if rt, ok := m.chasmRegistry.TaskByID(ct.Info.GetTypeId()); ok { | ||
| ct.TaskGroup = rt.TaskGroup() |
There was a problem hiding this comment.
can we move this logic into GrouperStateMachineNamespaceIDAndDestination implementation? Then only outbound queue will be doing this look up, and we don't need to change general persistence layer logic.
| NexusLink nexus.Link | ||
| } | ||
|
|
||
| const TaskGroupName = "nexusoperations" |
There was a problem hiding this comment.
Might as well use the name of the package
| const TaskGroupName = "nexusoperations" | |
| const TaskGroupName = "nexusoperation" |
But since this is going into persistence, I would consider shortening it to just nexus.
There was a problem hiding this comment.
it's not persisted based on my reading of the PR.
| func(ctx context.Context, ref ComponentRef, attrs TaskAttributes, task any) error { | ||
| return handler.Discard(ctx, ref, attrs, task.(T)) | ||
| }, | ||
| handler.TaskGroup(), |
There was a problem hiding this comment.
If you're not going to call this at runtime (contrary to what we discussed), I would 100% make it a registration option. Let's just go with registration option as @yycptt suggests.
S15
force-pushed
the
taskgroups-for-chasm
branch
from
aab4396 to
af6e0a2
Compare
| logger, | ||
| metricsHandler, | ||
| factory, | ||
| chasmTaskGroupPostProcessor(f.ChasmRegistry), |
There was a problem hiding this comment.
I wasn't able to convince myself doing this in GrouperStateMachineNamespaceIDAndDestination would cover e.g. OutboundTaskPredicate.Test where StateMachineTaskGroup is called directly, after tasks are loaded. So this seemed like the earliest after loading, before the tasks are really used, and also outside general persistence.
if y'all happen to know that the predicate testing isn't a concern, then I can move this again.
3 participants
What changed?
Adds a TaskGroup identifier for chasm side-effect handlers, threaded through registered tasks.
Why?
Enables chasm tasks to lookup their TaskGroup after being deserialized, so like-tasks can be grouped together.
How did you test it?