-
Notifications
You must be signed in to change notification settings - Fork 758
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
Add close time in mutable state #2917
Conversation
} else { | ||
closeTime = completionEvent.GetEventTime() | ||
} | ||
closeTime = executionInfo.GetCloseTime() |
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.
I think we still need the fallback logic for at least one release?
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.
Shall we call mutableState.GetWorkflowCloseTime as well here? or we don't care about compatibility for admin commands?
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.
Cannot get the mutable state in admin service. I think I still need your get completion event here for compatibility.
} else { | ||
closeTime = completionEvent.GetEventTime() | ||
} | ||
closeTime = executionInfo.GetCloseTime() |
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.
Shall we call mutableState.GetWorkflowCloseTime as well here? or we don't care about compatibility for admin commands?
* Add close time in mutable state
What changed?
Add close time in mutable state
Why?
In some code path, we don't need to fetch complete event to get the close time.
How did you test it?
Unit tests
Potential risks
Is hotfix candidate?