-
Notifications
You must be signed in to change notification settings - Fork 15
Feat cross app wf #26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
task/activity.go
Outdated
| type callActivityOptions struct { | ||
| rawInput *wrapperspb.StringValue | ||
| retryPolicy *RetryPolicy | ||
| AppID string |
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 is this AppID and not Target or similar?
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.
I left the OrchestrationCtx AppID as-is, and did not change it to sourceAppID. I did however change this to be targetAppID. I prefixed it with target to be clear its the target, not a source per say, but also left AppID to be clear what the 'target' is.
As of now its used like:
err := ctx.CallActivity("ProcessData",
task.WithActivityInput(input),
task.WithAppID(appID2)).
Await(&output)
lmk if you think it would be clearer to use task.WithTargetAppID(appID2)) or task.OnAppID(appID2) instead
| // Extract source AppID from HistoryEvent Router if this is ExecutionStartedEvent | ||
| if ctx.AppID == "" && e.GetRouter() != nil && e.GetRouter().GetSource() != "" { | ||
| ctx.AppID = e.GetRouter().GetSource() | ||
| } |
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.
What are we doing here?
Why are we checking whether AppID is an empty string?
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.
I tweaked the if statement, but I assigned ctx.AppID during the ExecutionStarted event because that’s when we can be sure we’re handling the current orchestration — not a parent or child in the call stack.
While router metadata is technically available on every history event, setting AppID earlier (for ex: in NewOrchestrationContext()) could be misleading in the presence of sub-orchestrations. To do that cleanly, I’d either need to pass AppID into NewOrchestrationContext (which would require changes across multiple ExecuteOrchestrator implementations in backend/orchestration.go, client/worker_grpc.go, and azurefunc/middleware.go) or scan through oldEvents and newEvents, which felt fragile and error-prone.
Open to suggestions if there's a cleaner place you think it belongs — happy to tweak!
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
expose cross app functionality in durabletask and update contributing docs with how to update protos
Implementation for this proposal