Skip to content
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

Gantt render stop early before the completion of the Execution #6953

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

rajatsingh23
Copy link
Contributor

@rajatsingh23 rajatsingh23 commented Jan 27, 2025

Closes #6913

What changes are being made?

  1. the el-card is rendered only if execution.taskRunList is available
  2. The tasks computed property is modified to populate its value when taskRunList is available.
  3. Execution Updates:
    • A debouncing mechanism has been added in the execution watcher to prevent multiple renders when the execution object is updated multiple times in quick succession.
    • The paint method is now called by the execution watcher when the component has received new data and the execution is not in a SUCCESS state.
    • The compute method is now called by the execution watcher when the component has received new data and the execution is in a SUCCESS state.
  4. The forEachItemsTaskRunIds watcher is now only updating the state when the value has changed.
  5. The computeSeries and computeDates methods are now being executed only if the tasks computed property has a value to be used.
  6. In Duration.vue, the histories watcher makes sure that the duration is also calculated at the initial render.

@tchiotludo tchiotludo force-pushed the Execution-Stop-Early branch from 743f9bd to f691f7e Compare January 27, 2025 17:56
@brian-mulier-p brian-mulier-p merged commit e4016eb into kestra-io:develop Jan 28, 2025
3 checks passed
@brian-mulier-p
Copy link
Member

In the end I got rid of the real-time notion as it complexifies things too much. Ty for your contribution!

@rajatsingh23
Copy link
Contributor Author

Hey @brian-mulier-p, I just reproduced the issue with your changes in my PR and I saw that it did not solve the issue.

@brian-mulier-p
Copy link
Member

That's really weird because I tried and reproduced before any changes and after mine I didn't reproduce anymore... Do you have a sample flow? Anyway we have to find a way to simplify the Gantt chart that's why I made those changes but maybe there's something more to do then... But the Gantt chart component was getting too complex and too painful to maintain.

@rajatsingh23
Copy link
Contributor Author

Hey @brian-mulier-p,

I just checked and found that my local repo and Docker weren’t updated properly. After updating, everything seems to be working fine now. However, there’s an issue in Duration.vue. The duration is not rendering as expected according to the issue statement. Upon reviewing the code, I found that the logic used in the watch method for histories contains an if statement: if (oldValue[0].date !== newValue[0].date). This condition is never true because the dates are always the same.

Instead, we can check oldValue.length !== newValue.length. This would ensure that the date renders as desired.

Would it be okay if I create a PR to solve this issue?

Thanks!

@brian-mulier-p
Copy link
Member

Ok no worries, for the Duration.vue I understand your logic however since the paint method is basically an auto-refresher till the state is no longer a running one, I can't see a case where it would fail. The watcher is here for cases where Vue reuses the same component but the props got changed so we can target a new durations, I'm not sure it's a wrong condition in such case so idk.
But if you find a case where there is an issue please share your reproducer and we can discuss workarounds 👍

@rajatsingh23
Copy link
Contributor Author

Hey @brian-mulier-p,

I tested further and was able to reproduce the issue. Here’s a video demonstrating the problem:

Screen.Recording.2025-02-04.234429.mp4

From what I observed, the duration does not update as expected in certain cases. This seems to confirm that the current watcher condition (if (oldValue[0].date !== newValue[0].date)) does not always trigger correctly. However, replacing it with oldValue.length !== newValue.length ensures the duration updates as intended.

Here's the video demonstration after changes:

Screen.Recording.2025-02-04.235043.mp4

Let me know your thoughts!

@brian-mulier-p
Copy link
Member

brian-mulier-p commented Feb 4, 2025

Ok understood, you can provide a PR and assign me as reviewer, thanks for this deep analysis 👍

rajatsingh23 added a commit to rajatsingh23/kestra that referenced this pull request Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

SSE for execution stop too soon before the end of the execution
2 participants