Uh oh!
There was an error while loading. Please reload this page.
[Standalone Activity] More cleanup - #675
Conversation
Uh oh!
There was an error while loading. Please reload this page.
8021ebb to
727283aCompareCo-authored-by: Fred Tzeng <41805201+fretz12@users.noreply.github.com>
ca0715a to
9ce001eCompare
dandavison
left a comment
There was a problem hiding this comment.
I made a few suggestions for improving the doc comments.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Dan Davison <dandavison7@gmail.com>
a189835 to
a6a28b9Compare
cretz
left a comment
There was a problem hiding this comment.
Some notes are related to past reviews that weren't caught then
| // More detailed breakdown of ACTIVITY_EXECUTION_STATUS_RUNNING. | ||
| temporal.api.enums.v1.PendingActivityState run_state = 5; | ||
| temporal.api.taskqueue.v1.TaskQueue task_queue = 6; |
There was a problem hiding this comment.
Any reason why this is a task queue object here but in the list info message it is a task queue string? Should we be consistent in our task queue representation?
There was a problem hiding this comment.
This is flattened out ActivityOptions, for workflow.PendingActivityInfo we also use the struct.
I can see that we would want to use the struct consistently for ActivityListInfo too though.
| int32 attempt = 9; | ||
| int32 maximum_attempts = 10; | ||
| int32 attempt = 15; | ||
| int32 maximum_attempts = 16; |
There was a problem hiding this comment.
Is this different than what's in retry policy? Can some docs be added to this field to explain it a bit? (I know this isn't part of this review, but it stood out)
There was a problem hiding this comment.
Ah, that's oversight by me. I copied this originally from PendingActivityInfo but that has duplication with ActivityOptions now. I am really regretting that we ever introduced ActivityOptions into the API.
We can avoid the duplication here at least.
| google.protobuf.Timestamp expiration_time = 12; | ||
| google.protobuf.Timestamp expiration_time = 18; | ||
| // Time when the activity transitioned to a closed state. | ||
| google.protobuf.Timestamp close_time = 19; |
There was a problem hiding this comment.
For list we are providing a helper field of execution_duration that is close_time - scheduled_time. Any reason to do that there and not here? Should we remove from list side?
In another review it was promised there would be a helper that confirmed that list would not contain more than describe. Are there still plans to write it?
There was a problem hiding this comment.
I don't think we have that tracked FTR. But if we do this for activities, we should also do it for schedules which takes the exact same approach.
There was a problem hiding this comment.
Unsure, was just told there would be a tool that would keep me from having to constantly go over these PRs with a fine tooth comb to catch mistakes.
Regardless, to the part of the comment about this execution_duration field, can we be consistent there?
There was a problem hiding this comment.
I'm on the fence if it's worth adding this field to ActivityInfo, I'm not even sure we need it in ActivityListInfo, it's available as a system search attribute, which is important to use as a query but not so important to reflect in the results, since it's really just subtracting timestamps.
| google.protobuf.Timestamp expiration_time = 12; | ||
| google.protobuf.Timestamp expiration_time = 18; | ||
| // Time when the activity transitioned to a closed state. | ||
| google.protobuf.Timestamp close_time = 19; |
There was a problem hiding this comment.
Past/present tense inconsistency between scheduled and close terms
There was a problem hiding this comment.
close_time the term WorkflowExecutionInfo uses, and scheduled_time is from PendingActivityInfo.
Should we be consistent here even though we won't be consistent with those other two structs?
There was a problem hiding this comment.
Up to you, just noticed them inconsistent in this same message
There was a problem hiding this comment.
I'll make it consistent here but the UI folks will probably not be happy with the inconsistency with other fields in PendingActivityInfo.
Uh oh!
There was an error while loading. Please reload this page.
**What changed?** - Removed all references to `ActivityOptions` - Add request_id to `TerminateActivityExecution` - Add `close_time` to `ActivityInfo` --------- Co-authored-by: Fred Tzeng <41805201+fretz12@users.noreply.github.com> Co-authored-by: Dan Davison <dandavison7@gmail.com>
**What changed?** - Removed all references to `ActivityOptions` - Add request_id to `TerminateActivityExecution` - Add `close_time` to `ActivityInfo` --------- Co-authored-by: Fred Tzeng <41805201+fretz12@users.noreply.github.com> Co-authored-by: Dan Davison <dandavison7@gmail.com>
**What changed?** - Removed all references to `ActivityOptions` - Add request_id to `TerminateActivityExecution` - Add `close_time` to `ActivityInfo` --------- Co-authored-by: Fred Tzeng <41805201+fretz12@users.noreply.github.com> Co-authored-by: Dan Davison <dandavison7@gmail.com>
**What changed?** - Removed all references to `ActivityOptions` - Add request_id to `TerminateActivityExecution` - Add `close_time` to `ActivityInfo` --------- Co-authored-by: Fred Tzeng <41805201+fretz12@users.noreply.github.com> Co-authored-by: Dan Davison <dandavison7@gmail.com>
What changed?
ActivityOptionsTerminateActivityExecutionclose_timetoActivityInfo