-
Notifications
You must be signed in to change notification settings - Fork 65
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 duration to cached steps #2967
base: main
Are you sure you want to change the base?
Conversation
duration is computed using corresponding job's started_at time
not sure it's needed.....
…nse and store it in JobResult
@@ -147,3 +149,7 @@ def test_serialize_and_truncate_does_not_raise(obj: Any, max_bytes: int, expecte | |||
def test_serialize_and_truncate_raises(obj: Any, max_bytes: int) -> None: | |||
with pytest.raises(SmallerThanMaxBytesError): | |||
serialize_and_truncate(obj=obj, max_bytes=max_bytes) | |||
|
|||
|
|||
def test_get_duration() -> None: |
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.
idk if that's even needed...
@@ -107,6 +107,7 @@ def run_job(self) -> JobResult: | |||
"job_runner_version": self.job_runner_version, | |||
"is_success": False, | |||
"output": None, | |||
"duration": get_duration_or_none(self.job_info["started_at"]), |
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 is dumb but otherwise mypy warns that Optional[datetime]
is being passed instead of datetime
because in theory job_info["started_at"]
might be None if a job has not started yet.
@@ -115,6 +117,8 @@ def test_backfill(priority: Priority, app_config: AppConfig) -> None: | |||
assert job_result["is_success"] | |||
assert job_result["output"] is not None | |||
assert job_result["output"]["content"] == {"key": "value"} | |||
assert job_result["duration"] is not None |
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 is the only test that checks that duration is set as expected for successful jobs. should I test it anywhere else?
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.
sounds good to me, though the test could be named test_run_job_and_finish
though
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.
LGTM !
@@ -115,6 +117,8 @@ def test_backfill(priority: Priority, app_config: AppConfig) -> None: | |||
assert job_result["is_success"] | |||
assert job_result["output"] is not None | |||
assert job_result["output"]["content"] == {"key": "value"} | |||
assert job_result["duration"] is not None |
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.
sounds good to me, though the test could be named test_run_job_and_finish
though
Will close #2892
As suggested in #2908 (review), this adds duration field to cached responses. Duration is computed using
started_at
field of a corresponding job.Note that this PR adds new fields to major libcommon dtos
JobInfo
andJobResult
.