CHASM: improve support for implementing Terminate method#9351
CHASM: improve support for implementing Terminate method#9351yycptt merged 3 commits intotemporalio:mainfrom
Conversation
chasm/registrable_task.go
Outdated
| registry *Registry, | ||
| ) (bool, error) { | ||
| return validator.Validate( | ||
| augmentContextForComponent(ctx, component, registry), |
There was a problem hiding this comment.
we can do the same thing for registered tasks as well.
| // This is useful for propagating values needed for those processing logic but are not avaiable via the | ||
| // component's struct definition, such as configurations. | ||
| func WithContextValue( | ||
| keyVals map[any]any, |
There was a problem hiding this comment.
since the keyVals are the same across component instances, we don't really need to use context.Context.Value() to implement this which adds overhead. The chasmContext.Value() method can just check the registry for a key. The semantic will be different from context.Context though.
bergundy
left a comment
There was a problem hiding this comment.
Had some small suggestions but otherwise LGTM.
| // ContextWithValue returns a new Context with the given key-value pair added. | ||
| // Added key-value pairs will be accessible via the Value() method on the returned Context, | ||
| // and the behavior of the key-value pair is the same as context.Context.WithValue(). | ||
| func ContextWithValue[C Context](c C, key any, value any) C { |
There was a problem hiding this comment.
You could consider structuring this as the Go stdlib does this: https://cs.opensource.google/go/go/+/refs/tags/go1.26.0:src/context/context.go;l=727.
You should be able to do this with type valueCtx[C Context] struct.
There was a problem hiding this comment.
hmm... I tried this before but embedding C directly into the valueCtx struct is not allowed.
type valueCtx[C Context] struct {
C
ctx context.Context
}
that won't compile..... so valueCtx still can't sometimes implement Context and sometimes implement MutableContext...
There was a problem hiding this comment.
Hmmm you can wrap it with base C but that's not critical IMHO.
chasm/registrable_component.go
Outdated
| // | ||
| // This is useful for propagating values needed for those processing logic but are not avaiable via the | ||
| // component's struct definition, such as configurations. | ||
| func WithContextValue( |
There was a problem hiding this comment.
| func WithContextValue( | |
| func WithContextValues( |
There was a problem hiding this comment.
But I'm also wondering if we want a function that takes a context and returns a context here. What you have is fine for now though.
There was a problem hiding this comment.
Renamed.
if we want a function that takes a context and returns a context here
Not sure I follow the use case here as this one is a registration option. Happy to add one when we have a need for it.
There was a problem hiding this comment.
Yeah the idea is that you can inject a metrics handler that is bound to the current request's namespace instead of passing in bits to construct that handler for example:
temporal/chasm/lib/activity/activity.go
Lines 298 to 303 in 6875191
f5af4f6 to
7b7eb67
Compare
7b7eb67 to
3d3f033
Compare
* origin/main: CHASM: improve support for implementing Terminate method (#9351) Add testhooks package documentation (#9373) Improve re-usability of ringpop membership & PerNamespaceWorker (#9321) Fairness counter: fix heap bug in map counter (#9370) Avoid finalGC when ack level is zero (#9371) Fairness counter: persist top K keys (#9188) Flake Fix: In Reactivation Cache tests, wait for appropriate delays when confirming expected drainage status (#9352) Include transient and speculative WFT events in GetWorkflowExecutionHistoryResponse (#9325) Fix flaky test TestTransitionDuringTransientTask (#9356) Add per-workflow scheduler for history task processing (#9141) Populate currentAttemptScheduledTime on PollActivityTaskQueueResponse for standalone activities (#9333) Standalone activity heartbeating bug fix (#9354) Revert "Last part of making Nexus work OOTB" (#9343) Convert flake report from Python to Go (#9334) Do not enforce payload limits for system nexus endpoint (#9344)
…9351) ## What changed? - Define TerminableComponent and RootComponent interface - Make metrics handler available through chasm context - Add support for passing key value pairs with chasm context similar to context.Context - Add support for specifying key values pairs that will always be available in the Context when a component is accessed. - Add requestID to TerminateComponentRequest ## Why? - Improve support for library authors to implement the Terminate method. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [x] covered by existing tests - [ ] added new unit test(s) - [x] added new functional test(s)
…9351) ## What changed? - Define TerminableComponent and RootComponent interface - Make metrics handler available through chasm context - Add support for passing key value pairs with chasm context similar to context.Context - Add support for specifying key values pairs that will always be available in the Context when a component is accessed. - Add requestID to TerminateComponentRequest ## Why? - Improve support for library authors to implement the Terminate method. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [x] covered by existing tests - [ ] added new unit test(s) - [x] added new functional test(s)
…9351) ## What changed? - Define TerminableComponent and RootComponent interface - Make metrics handler available through chasm context - Add support for passing key value pairs with chasm context similar to context.Context - Add support for specifying key values pairs that will always be available in the Context when a component is accessed. - Add requestID to TerminateComponentRequest ## Why? - Improve support for library authors to implement the Terminate method. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [x] covered by existing tests - [ ] added new unit test(s) - [x] added new functional test(s)
What changed?
Why?
How did you test it?