-
Notifications
You must be signed in to change notification settings - Fork 78
Move WorkflowExecutionPauseInfo inside WorkflowExecutionExtendedInfo #666
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
bergundy
left a comment
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.
If we are updating visibility anyways when a workflow is paused, why not put this field in WorkflowExecutionInfo and make it available in the list API?
I'm also glad that schedules, standalone activities, and nexus operations decided to make a cleaner cut between what goes in list vs describe responses. It's much easier to avoid this confusion with the newer APIs.
Visibility will have a small set of these fields (workflow_id, reason) compared to describe (pause time, identity, reason). Describe may have more in future. So probably it's better to keep them separate.
How did they make it cleaner? Can it be done here as well? |
cretz
left a comment
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.
LGTM (this is what I was trying to discuss at #653 (comment))
Yeah. Now I see it. I didn't fully realize the intention behind that comment. Assumed you are just checking if visibility will be populated as well. |
bergundy
left a comment
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.
Talked to @gow offline. There is some pause information available in visibility but not all of the PauseInfo fields can be populated with the information we store there. For example, only the reason is available, but the request ID and identity aren't.
In the list response, we will return the TemporalPauseInfo system search attribute which is a multi-purpose attribute and not easily parsable, but you will be able to extract the reason from it.
We can store more unindexed information in visibility but chose not to do it (@gow can you explain why?).
Based on this decision, pause_info will only be available in the describe response, and if we ever want to add it to visibility, we will need to send duplicate information because of the mess that is WorkflowExecutionExtendedInfo.
I'm leaning towards putting the field in WorkflowExecutionInfo so it can be made available in the list response later and documenting that it is not available right now.
In any case, I am not blocking progress on this PR, just want to understand the reasoning behind not wanting to return this information in the list response, given that we do a visibility write every time that pause info changes.
In the internal discussions it was required that only pause reason be indexed in visibility (request ID, identity only to be shown in Describe). |
From an SDK POV, we won't take this approach I don't think, but we have inheritance/embedding there where we can "lift" properties which you don't have here. I don't think we should make assumptions about what future things we may build. It is acceptable to deprecate fields if they are duplicated elsewhere later, as it is for any other proto in the system. |
|
Sounds good. Let's go with this change. We can revisit the structure and reorganizing it later if needed. |
This PR moves
pause_infointoWorkflowExecutionExtendedInfo.In #653 I accidentally put pause_info in
WorkflowExecutionInfo. It should have been inWorkflowExecutionExtendedInfoNote: This was merged yesterday (and the corresponding server changes are not yet merged). There are no references or usage of the
WorkflowExecutionInfo.pause_infofield. So was not necessary to reserve the proto field.