Skip to content

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Pharaohsk
Copy link

@Pharaohsk Pharaohsk commented May 10, 2025

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

  1. Closes
    Have implicit activity context timeout start-to-close be relative to system time not server time #1930

  2. 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

  1. Any docs updates needed?

N/A

@Pharaohsk Pharaohsk requested a review from a team as a code owner May 10, 2025 14:35
@CLAassistant
Copy link

CLAassistant commented May 10, 2025

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@cretz
Copy link
Member

cretz commented May 12, 2025

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 StartedTime to match exactly what the server reports for the activity and this would break that expectation. The time the activity started is actually owned by the server and is not the same as the time the activity reached the worker.

@Pharaohsk
Copy link
Author

Pharaohsk commented May 12, 2025

我不确定将活动开始时间向后不兼容地更改为不再使用服务器报告的开始时间是否是#1930的正确答案。#1930指的是可以使用系统时间进行的超时计算。人们可能期望活动信息与StartedTime服务器报告的活动信息完全匹配,但这会打破这种期望。活动开始的时间实际上归服务器所有,并且与活动到达工作器的时间不同。

thanks for your reminder. I have modified it

started := task.GetStartedTime().AsTime()
started := time.Now()
Copy link
Member

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).

Copy link
Author

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

Copy link
Member

@cretz cretz left a 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants