smart-data-lake / sdl-visualization

Visualization for SDLB config
GNU General Public License v3.0
4 stars 1 forks source link

Prepare init phase #52

Closed Entouanes closed 1 year ago

Entouanes commented 1 year ago

What's new

pgruetter commented 1 year ago

Looks great! A few comments:

  1. As discussed, I think the duration next to the action name should be the whole duration, not only the exec phase: image
  2. I would argue that the default for the Phases filter should be "all" instead of only exec phase.
  3. A strange effect I noticed: When I select all 3 Phases, it correctly displays all phases. But if I then click on another browser window or Windows application and then go back to the SDLB UI, the filter is automatically reset.
zzeekk commented 1 year ago

Nice! My two cents on @pgruetter's points: 1) In my opinion the duration should be the sum of all intervals displayed, e.g. if Prep/Init/Exec is displayed it should be durationPrep + durationInit + duration Exec. If we would just display tstmpExecEnd - tstampPrepStart the value would be unusable as well... 2) i disagree, normally i'm only interested in exec phase. Showing all phases for large pipelines looks also quite complex and can be confusing.

pgruetter commented 1 year ago
  1. i disagree, normally i'm only interested in exec phase. Showing all phases for large pipelines looks also quite complex and can be confusing.

I see your point, especially for larger pipelines. Let's keep this default then.

Entouanes commented 1 year ago
  1. A strange effect I noticed: When I select all 3 Phases, it correctly displays all phases. But if I then click on another browser window or Windows application and then go back to the SDLB UI, the filter is automatically reset.

@pgruetter That's true, and this has to do with the relatively aggressive setting of the "React Query" library. Long story short, it probes the remote source for changes continuously as you are on the window. If it detect you were on another tab/application for even a short period, it re-triggers a fetch. Since I have not set the states to persist upon re-fetch, the whole page is reloaded and filters are reset. I suggest adding this to the backlog and defining the correct behavior we want.

  1. In my opinion the duration should be the sum of all intervals displayed, e.g. if Prep/Init/Exec is displayed it should be durationPrep + durationInit + duration Exec. If we would just display tstmpExecEnd - tstampPrepStart the value would be unusable as well...

@zzeekk @pgruetter I will implement the time for an action as the addition of the duration of each phases 👍