CHASM: Propagate ArchetypeID #8693
Conversation
yycptt
requested review from
bergundy,
lina-temporal,
prathyushpv,
xwduan and
yux0
| } | ||
|
|
||
| archetypeID := mutableState.ChasmTree().ArchetypeID() | ||
| if archetypeID != chasm.WorkflowArchetypeID { |
There was a problem hiding this comment.
Will it have backward compatibility issue? i.e. for some old workflow which dose not have archetype? And do we need any logic to prevent other Archetype not call this func or return early?
There was a problem hiding this comment.
Will it have backward compatibility issue?
No there shouldn't be compatibility issue. An empty chasm tree will also return workflow archetypeID.
do we need any logic to prevent other Archetype not call this func or return early?
There's a check early in the function to return error when version history is empty, which applies to all non workflow archetypes.
| // Should be called business_id, but to maintain backward compatibility | ||
| // with old type definition used in migration workflows. | ||
| string workflow_id = 1; |
There was a problem hiding this comment.
Not sure sure why we can't call this business ID. Can you explain?
There was a problem hiding this comment.
We used to return commonpb.WorkflowExecution in activity result, which is encoded by the default dataconverter via protojson, so can't directly change the field name...
I can do string business_id = 1 [json_name = "workflowId"]; though, I believe that's still compatible. WDYT?
There was a problem hiding this comment.
I changed it to string business_id = 1 [json_name = "workflowId"]
service/history/workflow/context.go
Outdated
| tag.ArchetypeID(c.archetypeID), | ||
| tag.NewUInt32("actual-archetype-id", actualArchetypeID), |
There was a problem hiding this comment.
My concern with these logs is that they're going to be very hard to reason about. Ideally we would resolve the archetype names before logging and emitting other operational information such as metrics and internal errors.
There was a problem hiding this comment.
Updated the logic to do a best effort convert to archetype names.
| ArchetypeId: chasm.UnspecifiedArchetypeID, | ||
| } | ||
|
|
||
| nsDivisionPayload, ok := e.SearchAttributes.IndexedFields[sadefs.TemporalNamespaceDivision] |
There was a problem hiding this comment.
Is this ensured to not be nil?
There was a problem hiding this comment.
Updated to use GetIndexedFields() and performs nil check on the map.
tests/chasm_test.go
Outdated
|
|
||
| // TODO: More tests here... | ||
|
|
||
| // TODO: add a test to make sure workflow can run successfully when chasm is enabled. |
There was a problem hiding this comment.
That's technically already covered by the callback tests.
|
Also requires: #8704 |
3 participants
What changed?
Why?
How did you test it?