-
Notifications
You must be signed in to change notification settings - Fork 239
Use system time as started time #1952
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
base: master
Are you sure you want to change the base?
Conversation
|
I am not sure backwards-incompatibly changing the activity start time to no longer be the server-reported start time is the correct answer to #1930. #1930 refers to the timeout calculation which can be done with system time. People may expect the activity info's |
thanks for your reminder. I have modified it |
internal/activity.go
Outdated
started := task.GetStartedTime().AsTime() | ||
started := time.Now() |
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 don't think we should change this variable, just leave it as it was and adjust calculateActivityDeadline
to no longer accept a started
time and just use time.Now()
within it. It's effectively the same thing, but I think it's clearer not change anything in this function (except it'll be one less parameter to calculateActivityDeadline
).
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.
thanks for your review. I have modified it
Use time.Now as started in calculateActivityDeadline
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.
This looks good to me, @Quinn-With-Two-Ns?
What was changed
Use system time as started time
Why?
Have implicit activity context timeout start-to-close be relative to system time not server time
Checklist
Closes
Have implicit activity context timeout start-to-close be relative to system time not server time #1930
How was this tested:
The time is not consistent across hosts in the construct cluster.
The local time of the host on which the construct executes the activity has exceeded the timeout set by the server.
#1912
N/A