Only refresh active dags on dags page

[hankehly]

closes: #23949
related: #22900

Summary

This PR changes the behavior of the auto-refresh feature on the dags page. Auto-refresh currently targeted all dags. This PR modifies the behavior to only targets active (ie. unpaused) dags.

A few important notes:

• Clicking the refresh button (the circular arrow icon) at the top right of the page still refreshes all dags.
• If there are no active dags, and the "auto-refresh" switch is enabled, no request is sent and the switch stays enabled.

This behavior makes the most sense to me, but I have no problem changing it if requested.

Demo

issue-23949.mp4

[hankehly]

refreshDagStats uses jQuery, so I brought it inside the page load callback with the other init functions.

[hankehly]

getAllDagIds and getActiveDagIds return an array of dag ids. Passing either function to handleRefresh or refreshDagStats let's the developer specify all / just a subset of dag ids to use in the HTTP request.

[hankehly]

I added a getDagIds argument here to keep the behavior consistent with the handleRefresh function. It's unused, so I can remove it if requested.

[bbovenzi]

As in my comment below, let's remove it if we're not using it. No need for excess code.

[bbovenzi]

I'm not a big fan of passing functions and then reassigning those arguments in refreshDagStats() and handleRefresh(). It's a lot of extra logic to follow. I think it would be clearer to pass a refreshAll boolean variable, and even combine getDagIds and getAllDagIds into a single function.

Therefore it would look more like:

function handleRefresh(refreshAllDags = false) {
  const dagIds = getDagIds(refreshAllDags);
  dagIds.forEach((dagId) => {
    params.append('dag_ids', dagId);
  });

[bbovenzi]

should this be called getDagStats since we only call it once? We don't ever call it with getDagIds either

[hankehly]

@bbovenzi Thanks for the review!

I made changes based on your feedback.

I'm not a big fan of passing functions and then reassigning those arguments in refreshDagStats() and handleRefresh(). It's a lot of extra logic to follow. I think it would be clearer to pass a refreshAll boolean variable, and even combine getDagIds and getAllDagIds into a single function.

Thanks for your insight. While I think passing functions can make the code more extensible/tolerant to change (kind of like the strategy pattern in OOP) it definitely isn't required to complete this task.

I do think passing functions has benefits (like the following), so I hope we can consider this approach in the future, maybe in a different context.

benefits in this case:

• by passing in a different function to handleRefresh, we can modify its behavior without changing its implementation/tests
• writing out something like handleRefresh(getAllDagIds) gives a little more information about which dags are being targeted as opposed to handleRefresh(true) (if only we could write handleRefresh(activeOnly=true))

Thanks again.

[bbovenzi]

Thanks for your insight. While I think passing functions can make the code more extensible/tolerant to change (kind of like the strategy pattern in OOP) it definitely isn't required to complete this task.

I do think passing functions has benefits (like the following), so I hope we can consider this approach in the future, maybe in a different context.

benefits in this case:

• by passing in a different function to handleRefresh, we can modify its behavior without changing its implementation/tests
• writing out something like handleRefresh(getAllDagIds) gives a little more information about which dags are being targeted as opposed to handleRefresh(true) (if only we could write handleRefresh(activeOnly=true))

My main issue was reassigning the parameter.
I agree that just true isn't too descriptive. We can turn the props into an object handleRefresh({ activeOnly = true }).

[hankehly]

@bbovenzi I went ahead and changed the function signatures. handleRefresh({ activeDagsOnly: true }) is definitely more clear.

[bbovenzi]

Looking better! Although, it looks like we have 2 errors from yarn lint. Fix those and I think this should be good to go

[bbovenzi]

nice work!