Attach current Build ID to workflow activations #619
Conversation
core/src/protosext/mod.rs
Outdated
| @@ -147,6 +149,11 @@ impl WorkflowActivationExt for WorkflowActivation { | |||
| variant: Some(workflow_activation_job::Variant::QueryWorkflow(qr)) | |||
| }] if qr.query_id == LEGACY_QUERY_ID) | |||
| } | |||
| fn attach_build_id_if_needed(&mut self, build_id: &str) { | |||
There was a problem hiding this comment.
This is a bit confusing, why would this logic be on a proto struct, it doesn't know when attaching is needed.
I would put it in the worker logic before it hands over the activation or better yet when the activation is created to avoid mutating unnecessarily.
There was a problem hiding this comment.
It can't really be when the activation is created without passing the build id down a few layers where it's not currently needed (and would get cloned a bunch of times). I can just inline the method where it's used though.
There was a problem hiding this comment.
Inlined it w/ comment
There was a problem hiding this comment.
LGTM.
Lang impl note: In Python and .NET we have tried to make properties on the info immutable for the life of the run (even if the contents of that property's object may change). So we'll either be exposing this as a method on the info (Python only if so), or top-level on the workflow context. Other languages however may not have this immutability rule.
Sushisource
force-pushed
the
expose-build-id-wfinfo
branch
from
ce07a2d to
9c5796e
Compare
What was changed
Add current build id as a field on workflow activations
Why?
So users can tell during replay what build id was used to process the current task.
Checklist
Closes [Feature Request] Expose build ID via WorkflowInfo. features#253
How was this tested:
Added unit test
Any docs updates needed?