Add CHASM Visibility component to CHASM Workflow#10434
Conversation
rodrigozhou
force-pushed
the
rodrigozhou/chasm-wf-vis
branch
from
cbc6ddc to
fc07bc7
Compare
yycptt
left a comment
yycptt
left a comment
There was a problem hiding this comment.
PR lgtm but want to understand a bit more on the migration plan. Looks like with this PR we are not doing dual write for custom SA/Memo yet, but workflow will generate both old visibility task and also chasm visibility task?
| } | ||
|
|
||
| // CustomSearchAttributes returns the custom search attributes. | ||
| func (w *Workflow) CustomSearchAttributes(ctx chasm.Context) map[string]*commonpb.Payload { |
There was a problem hiding this comment.
where/how will the following four methods be used?
There was a problem hiding this comment.
It will be used in the mutable state. I'll have a PR using those soon.
|
|
||
| // SearchAttributes returns the predefined search attributes set in the underlying mutable state. | ||
| func (w *Workflow) SearchAttributes(ctx chasm.Context) []chasm.SearchAttributeKeyValue { | ||
| searchAttributes, err := w.GetPredefinedSearchAttributes() |
There was a problem hiding this comment.
do we have to deep copy/clone the SAs here?
There was a problem hiding this comment.
We need for backwards compatibility if we need to read search attributes from the mutable state that doesn't contain the metadata type. It's only making a deep copy when necessary, ie., when it doesn't contain the metadata type. Otherwise, it makes a shallow copy.
|
Not related to content of the PR, but what happened to the CODEREVIEWERS requirement? |
The PR is not targeting |
| } | ||
|
|
||
| // GetPredefinedSearchAttributes retrieves the predefined search attributes from the underlying mutable state. | ||
| func (m MSPointer) GetPredefinedSearchAttributes() (map[string]VisibilityValue, error) { |
There was a problem hiding this comment.
maybe I need more context for this, will predefined search attributes end up getting moved to the Workflow Component's state? I guess I am in general having the same question as Yichao; do visibility tasks end up getting duplicated (MS backed and CHASM backed) in this current implementation?
I guess when I say duplicate here, I mean CHASM visibility tasks end up writing both custom and predefined search attributes, whereas the legacy visibility task only reads mutable state, and writes only predefined search attributes?
There was a problem hiding this comment.
also for naming purposes, these predefined search attributes include Predefined and System search attributes right?
There was a problem hiding this comment.
Predefined here means really only the predefined search attributes that are stored together with custom search attributes.
At this moment, the idea is that all predefined search attributes are stored in the mutable state, and CHASM Workflow extracts the values from there. Not stored twice.
For custom search attributes, it would be duplicated. More details in a follow up PR. This PR is not really adding any tasks since it doesn't introduce any changes to the CHASM Visibility component.
| return ms.executionState | ||
| } | ||
|
|
||
| func (ms *MutableStateImpl) ExecutionStateUpdated() bool { |
There was a problem hiding this comment.
what's the purpose of this method?
There was a problem hiding this comment.
Execution state is managed in the mutable state currently, and the CHASM Workflow needs to know if the execution state changed to generate CHASM visibility task.
rodrigozhou
force-pushed
the
rodrigozhou/chasm-vis-text
branch
from
f53f7e1 to
1322d4b
Compare
rodrigozhou
force-pushed
the
rodrigozhou/chasm-wf-vis
branch
from
fc07bc7 to
c4f86c1
Compare
3 participants
What changed?
Add CHASM Visibility component to CHASM Workflow
Why?
Support storing custom search attributes and memo in CHASM Workflow.
How did you test it?
Potential risks