Add workflow expiration and run timestamps to execution info#505
Add workflow expiration and run timestamps to execution info#505ychebotarev merged 6 commits intomasterfrom
Conversation
| } | ||
|
|
||
| // Holds all the extra information about workflow execution. | ||
| message WorkflowExecutionExtendedInfo { |
There was a problem hiding this comment.
btw, it will be a good time to ask for more info.
Example:
I'm adding cancel_requested - we want to do that for a long time, but also don't want to add it to ES
There was a problem hiding this comment.
it will be a good time to ask for more info
Can I have original_start_time?
There was a problem hiding this comment.
You can.
But start time in workflowExecutionInfo ->is<- the original start time.
There was a problem hiding this comment.
Oh, my mistake. From the thread before at #505 (comment), I was under the impression that whatever time was needed to add to execution_timeout to make execution_expiration_time was not available. Can we add whatever that time is? Or is execution_expiration_time always equal to start_time from execution info + execution_timeout from execution config?
There was a problem hiding this comment.
whatever time was needed to add to execution_timeout to make execution_expiration_time was not available
It is NOT available right now in DescribeWorkflow response. Only "StartTime", which is basically "original_start_time"
That is why I added "last_reset_time".
In theory "execution_expiration_time" = max("start_time", "last_reset_time") + "execution_expiration_timeout"
On practice - we have a case when we still manage to make a bug with this seemingly simple expiration time calculations... :) (for activities, but in a somewhat tricky manner)
So if we have some bug, or change the way expiration time is calculated - without providing exact value it will not be visible to the user.
Thus I prefer to explicitly return "execution_expiration_time". This way we return whatever we use, right or wrong, old or new.
There was a problem hiding this comment.
In theory "execution_expiration_time" = max("start_time", "last_reset_time") + "execution_expiration_timeout"
Great, this works for me. Nothing else blocking from my POV.
On practice - we have a case when we still manage to make a bug with this seemingly simple expiration time calculations...
I am a bit curious about this. Are you saying we have a bug today, or are you saying it's possible that "execution_expiration_time" = max("start_time", "last_reset_time") + "execution_expiration_timeout" may not always be true?
Thus I prefer to explicitly return "execution_expiration_time"
This is fine, I don't mind the redundant data, though a bit concerned about the idea that it may change or that we may introduce bugs.
| } | ||
|
|
||
| // Holds all the extra information about workflow execution. | ||
| message WorkflowExecutionExtendedInfo { |
There was a problem hiding this comment.
it will be a good time to ask for more info
Can I have original_start_time?
| } | ||
|
|
||
| // Holds all the extra information about workflow execution. | ||
| message WorkflowExecutionExtendedInfo { |
There was a problem hiding this comment.
It is a bit unclear that "info" is in visibility and "extended info" is not. Maybe at least clarify at the top of the proto so people know the difference between the two and why some fields are here and some are there.
There was a problem hiding this comment.
Added some comments.
There was a problem hiding this comment.
Can you have @alexshtin or someone from server re-approve? This changed quite a bit since original approval (new type, moved out of visibility, etc).
|
Yes, it looks very different from what it was. I wish we started that new |
_**READ BEFORE MERGING:** All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted._ <!-- Describe what has changed in this PR --> **What changed?** Add new structure -WorkflowExecutionExtendedInfo That structure has few fields: * execution timeout time * run timeout time * last reset time * cancel requested (the last one from #339) <!-- Tell your future self why have you made these changes --> **Why?** Per customer request. Execution timeout timestamp may change after workflow reset, and in this case customers have no idea when it will fire. WorkflowExecutionInfo is reused in other server calls, like ListWorkflow, etc. Because of that we either add any new field to ES, or it will be nil which is bad user experience. <!-- Are there any breaking changes on binary or code level? --> **Breaking changes** No <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> **Server PR** N/A
5 participants
READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.
What changed?
Add new structure -WorkflowExecutionExtendedInfo
That structure has few fields:
(the last one from #339)
Why?
Per customer request.
Execution timeout timestamp may change after workflow reset, and in this case customers have no idea when it will fire.
WorkflowExecutionInfo is reused in other server calls, like ListWorkflow, etc.
Because of that we either add any new field to ES, or it will be nil which is bad user experience.
Breaking changes
No
Server PR
N/A