You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/07/01 01:27:00 UTC
[GitHub] [airflow] hankehly opened a new pull request, #24770: Only refresh active dags on dags page
hankehly opened a new pull request, #24770:
URL: https://github.com/apache/airflow/pull/24770
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
https://user-images.githubusercontent.com/11639738/176803378-b42d2bf7-6a4f-4cf1-b592-2107d105c479.mp4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #24770:
URL: https://github.com/apache/airflow/pull/24770#issuecomment-1180465539
Looking better! Although, it looks like we have 2 errors from `yarn lint`. Fix those and I think this should be good to go
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #24770:
URL: https://github.com/apache/airflow/pull/24770#discussion_r914142822
##########
airflow/www/static/js/dags.js:
##########
@@ -328,24 +325,32 @@ function taskStatsHandler(error, json) {
});
}
-if (encodedDagIds.has('dag_ids')) {
- // dags on page fetch stats
- d3.json(blockedUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, blockedHandler);
- d3.json(lastDagRunsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, lastDagRunsHandler);
- d3.json(dagStatsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, dagStatsHandler);
- d3.json(taskStatsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, taskStatsHandler);
-} else {
- // no dags, hide the loading dots
- $('.js-loading-task-stats').remove();
- $('.js-loading-dag-stats').remove();
+function refreshDagStats(getDagIds) {
+ if (typeof getDagIds !== 'function') {
+ getDagIds = getAllDagIds;
Review Comment:
As in my comment below, let's remove it if we're not using it. No need for excess code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #24770:
URL: https://github.com/apache/airflow/pull/24770#issuecomment-1179566496
> 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 })`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
hankehly commented on PR #24770:
URL: https://github.com/apache/airflow/pull/24770#issuecomment-1179645081
@bbovenzi I went ahead and changed the function signatures. `handleRefresh({ activeDagsOnly: true })` is definitely more clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi merged pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
bbovenzi merged PR #24770:
URL: https://github.com/apache/airflow/pull/24770
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on a diff in pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
hankehly commented on code in PR #24770:
URL: https://github.com/apache/airflow/pull/24770#discussion_r919511657
##########
airflow/www/static/js/dags.js:
##########
@@ -428,17 +442,24 @@ function refreshDagRuns(error, json) {
});
}
-function handleRefresh() {
+function handleRefresh({ activeDagsOnly = false } = {}) {
+ const dagIds = getDagIds({ activeDagsOnly });
+ const params = new URLSearchParams();
+ dagIds.forEach(((dagId) => {
Review Comment:
Embarrassing, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on a diff in pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
hankehly commented on code in PR #24770:
URL: https://github.com/apache/airflow/pull/24770#discussion_r911548915
##########
airflow/www/static/js/dags.js:
##########
@@ -485,6 +515,8 @@ $(window).on('load', () => {
$('body').on('mouseout', '.has-svg-tooltip', () => {
hideSvgTooltip();
});
+
+ refreshDagStats();
Review Comment:
`refreshDagStats` uses jQuery, so I brought it inside the page load callback with the other init functions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #24770:
URL: https://github.com/apache/airflow/pull/24770#discussion_r914142359
##########
airflow/www/static/js/dags.js:
##########
@@ -328,24 +325,32 @@ function taskStatsHandler(error, json) {
});
}
-if (encodedDagIds.has('dag_ids')) {
- // dags on page fetch stats
- d3.json(blockedUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, blockedHandler);
- d3.json(lastDagRunsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, lastDagRunsHandler);
- d3.json(dagStatsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, dagStatsHandler);
- d3.json(taskStatsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, taskStatsHandler);
-} else {
- // no dags, hide the loading dots
- $('.js-loading-task-stats').remove();
- $('.js-loading-dag-stats').remove();
+function refreshDagStats(getDagIds) {
Review Comment:
should this be called `getDagStats` since we only call it once? We don't ever call it with `getDagIds` either
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on a diff in pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
hankehly commented on code in PR #24770:
URL: https://github.com/apache/airflow/pull/24770#discussion_r911549359
##########
airflow/www/static/js/dags.js:
##########
@@ -447,7 +477,7 @@ function handleRefresh() {
function startOrStopRefresh() {
if ($('#auto_refresh').is(':checked')) {
refreshInterval = setInterval(() => {
- handleRefresh();
+ handleRefresh(getActiveDagIds);
Review Comment:
Only target active dags on auto-refresh interval.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on a diff in pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
hankehly commented on code in PR #24770:
URL: https://github.com/apache/airflow/pull/24770#discussion_r911553760
##########
airflow/www/static/js/dags.js:
##########
@@ -328,24 +325,32 @@ function taskStatsHandler(error, json) {
});
}
-if (encodedDagIds.has('dag_ids')) {
- // dags on page fetch stats
- d3.json(blockedUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, blockedHandler);
- d3.json(lastDagRunsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, lastDagRunsHandler);
- d3.json(dagStatsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, dagStatsHandler);
- d3.json(taskStatsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, taskStatsHandler);
-} else {
- // no dags, hide the loading dots
- $('.js-loading-task-stats').remove();
- $('.js-loading-dag-stats').remove();
+function refreshDagStats(getDagIds) {
+ if (typeof getDagIds !== 'function') {
+ getDagIds = getAllDagIds;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #24770:
URL: https://github.com/apache/airflow/pull/24770#discussion_r919011637
##########
airflow/www/static/js/dags.js:
##########
@@ -428,17 +442,24 @@ function refreshDagRuns(error, json) {
});
}
-function handleRefresh() {
+function handleRefresh({ activeDagsOnly = false } = {}) {
+ const dagIds = getDagIds({ activeDagsOnly });
+ const params = new URLSearchParams();
+ dagIds.forEach(((dagId) => {
Review Comment:
```suggestion
dagIds.forEach((dagId) => {
```
Looks like we have 1 too many parens here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
hankehly commented on PR #24770:
URL: https://github.com/apache/airflow/pull/24770#issuecomment-1179475253
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on a diff in pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
hankehly commented on code in PR #24770:
URL: https://github.com/apache/airflow/pull/24770#discussion_r911551706
##########
airflow/www/static/js/dags.js:
##########
@@ -328,24 +325,32 @@ function taskStatsHandler(error, json) {
});
}
-if (encodedDagIds.has('dag_ids')) {
Review Comment:
I wrapped this block of code in a function and call it on page load (see the bottom of the file)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on a diff in pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
hankehly commented on code in PR #24770:
URL: https://github.com/apache/airflow/pull/24770#discussion_r911549688
##########
airflow/www/static/js/dags.js:
##########
@@ -428,17 +433,42 @@ function refreshDagRuns(error, json) {
});
}
-function handleRefresh() {
+function getAllDagIds() {
+ const dagIds = $('[id^=toggle]').map(function () {
+ return $(this).data('dag-id');
+ }).get();
+ return dagIds;
+}
+
+function getActiveDagIds() {
+ const dagIds = $('[id^=toggle]').filter(':checked').map(function () {
+ return $(this).data('dag-id');
+ }).get();
+ return dagIds;
+}
+
+// To target a subset of dags, pass a function that returns an array
+// of dag ids. If no function is specified, all dags are targeted.
+function handleRefresh(getDagIds) {
+ if (typeof getDagIds !== 'function') {
+ getDagIds = getAllDagIds;
+ }
+ const params = new URLSearchParams();
+ getDagIds().forEach(dagId => {
+ params.append('dag_ids', dagId);
+ });
$('#loading-dots').css('display', 'inline-block');
- d3.json(lastDagRunsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, lastDagRunsHandler);
- d3.json(dagStatsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, refreshDagRuns);
- d3.json(taskStatsUrl)
- .header('X-CSRFToken', csrfToken)
- .post(encodedDagIds, refreshTaskStateHandler);
+ if (params.has('dag_ids')) {
Review Comment:
Don't send a request unless there are dags to refresh.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on a diff in pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
hankehly commented on code in PR #24770:
URL: https://github.com/apache/airflow/pull/24770#discussion_r911550349
##########
airflow/www/static/js/dags.js:
##########
@@ -428,17 +433,42 @@ function refreshDagRuns(error, json) {
});
}
-function handleRefresh() {
+function getAllDagIds() {
Review Comment:
`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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hankehly commented on a diff in pull request #24770: Only refresh active dags on dags page
Posted by GitBox <gi...@apache.org>.
hankehly commented on code in PR #24770:
URL: https://github.com/apache/airflow/pull/24770#discussion_r911549235
##########
airflow/www/static/js/dags.js:
##########
@@ -506,7 +538,7 @@ $('.js-next-run-tooltip').each((i, run) => {
$('#auto_refresh').change(() => {
if ($('#auto_refresh').is(':checked')) {
// Run an initial refresh before starting interval if manually turned on
- handleRefresh();
+ handleRefresh(getActiveDagIds);
Review Comment:
Only target active dags when the user enables auto-refresh.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org