Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Update startedAt timestamp only if not set #567

Merged
merged 2 commits into from
May 22, 2023

Conversation

pmahindrakar-oss
Copy link
Contributor

@pmahindrakar-oss pmahindrakar-oss commented May 19, 2023

TL;DR

Updation of the startedAt timestamp for same taskExecution happens in case of map tasks.
So when propeller send the TaskExecutionEvents for each mappedTask, admin records the event and if it finds it to be Running, then calls the addTaskStartedState function which update the startedAt timestamp.

This is fine to do in case of regular task but incase of mapped task, each mappedTask status causes an update to startedAt which is incorrect.
This PR fixes it by guarding it with if the values is not set

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

fixes flyteorg/flyte#3702

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

Signed-off-by: pmahindrakar-oss <[email protected]>
@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #567 (b0d2e25) into master (313ac29) will increase coverage by 1.49%.
The diff coverage is 100.00%.

❗ Current head b0d2e25 differs from pull request most recent head 9242d9a. Consider uploading reports for the commit 9242d9a to get more accurate results

@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
+ Coverage   58.46%   59.95%   +1.49%     
==========================================
  Files         168      168              
  Lines       16154    13216    -2938     
==========================================
- Hits         9444     7924    -1520     
+ Misses       5871     4453    -1418     
  Partials      839      839              
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/repositories/transformers/task_execution.go 76.11% <100.00%> (+2.32%) ⬆️

... and 154 files with indirect coverage changes

@pmahindrakar-oss
Copy link
Contributor Author

Integration tests failing on unrelated changes to this PR. Previous PR's aswell failing on the same issue

Copy link
Contributor

@hamersaw hamersaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative solution that would fix the existing conditional rather than just add another is to update this line to something like:

if existingTaskPhase != core.TaskExecution_RUNNING.String() && taskExecutionModel.StartedAt == nil {

I am OK with either fix.

@pmahindrakar-oss
Copy link
Contributor Author

@hamersaw the one i implemented i found it more unit testable aswell as since we are using this function at multiple places so wanted to make sure we define the behavior in the function itself to avoid any future calls causing the same regression

err := addTaskStartedState(input.Request, taskExecution, closure)

I will use the current approach for that reason. Let me know if you disagree

@pmahindrakar-oss pmahindrakar-oss merged commit 7915ae8 into master May 22, 2023
@pmahindrakar-oss pmahindrakar-oss deleted the fix-map-task-startedAt branch May 22, 2023 21:22
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* Update startedAt timestamp only if not set

Signed-off-by: pmahindrakar-oss <[email protected]>

* sort imports

Signed-off-by: pmahindrakar-oss <[email protected]>

---------

Signed-off-by: pmahindrakar-oss <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Mapped task overwrite the parent map task startedAt timestamp
3 participants