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/10/19 14:45:56 UTC
[GitHub] [airflow] BobDu opened a new pull request, #27141: Fix some bug in web ui dags list page (auto-refresh & jump search null state)
BobDu opened a new pull request, #27141:
URL: https://github.com/apache/airflow/pull/27141
Signed-off-by: BobDu <i...@bobdu.cc>
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
This is a fix for multiple smiles bug in web ui dags page, but need to modify the same file, so I create a PR.
related: #22900
1. after auto-refresh, none state task-run circle attr title lost.
Before fix,
After fix,
2. after auto-refresh, dag-run and task-run circle mouse over event lost.
Before fix,
After fix,
related: #26584
3. jump to right task-instance search url when click none state task-run circle
Before fix,
http://localhost:8080/taskinstance/list/?_flt_3_dag_id=tutorial&_flt_3_state=null
After fix,
http://localhost:8080/taskinstance/list/?_flt_3_dag_id=tutorial&_flt_8_state=
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] BobDu commented on a diff in pull request #27141: Fix some bug in web ui dags list page (auto-refresh & jump search null state)
Posted by GitBox <gi...@apache.org>.
BobDu commented on code in PR #27141:
URL: https://github.com/apache/airflow/pull/27141#discussion_r1001293687
##########
airflow/www/static/js/dags.js:
##########
@@ -204,33 +204,41 @@ function drawDagStatsForDag(dagId, states) {
});
g.append('svg:a')
- .attr('href', (d) => `${dagRunUrl}?_flt_3_dag_id=${dagId}&_flt_3_state=${d.state}`)
+ .attr('href', (d) => {
+ const params = new URLSearchParams();
+ params.append('_flt_3_dag_id', dagId);
+ /* eslint no-unused-expressions: ["error", { "allowTernary": true }] */
+ d.state ? params.append('_flt_3_state', d.state) : params.append('_flt_8_state', '');
Review Comment:
Sorry, we can't do this.
See, #26584
If d.state is null, use params `_flt_8_state` , mean `Is Null`, else use `_flt_3_state `, mean `Equal to`.
Of course, we can use `if ... else ...` statement block to shake off eslint exception.
But I still think use ternary expression maybe more suitable in it.
--
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] potiuk commented on pull request #27141: Fix some bug in web ui dags list page (auto-refresh & jump search null state)
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27141:
URL: https://github.com/apache/airflow/pull/27141#issuecomment-1336474365
> @jyotsa09
>
> Oh, this error is beacuse of support search none task in PR #26584 . But only charry-pick this PR in 2.4.3 I think, the only thing we can do now is probably wait 2.5.0 release.
Which is out.
--
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 #27141: Fix some bug in web ui dags list page (auto-refresh & jump search null state)
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #27141:
URL: https://github.com/apache/airflow/pull/27141#discussion_r1001857241
##########
airflow/www/static/js/dags.js:
##########
@@ -204,33 +204,41 @@ function drawDagStatsForDag(dagId, states) {
});
g.append('svg:a')
- .attr('href', (d) => `${dagRunUrl}?_flt_3_dag_id=${dagId}&_flt_3_state=${d.state}`)
+ .attr('href', (d) => {
+ const params = new URLSearchParams();
+ params.append('_flt_3_dag_id', dagId);
+ /* eslint no-unused-expressions: ["error", { "allowTernary": true }] */
+ d.state ? params.append('_flt_3_state', d.state) : params.append('_flt_8_state', '');
Review Comment:
Oh yes, my bad. I didn't realse the 3 vs the 8.
--
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] BobDu commented on pull request #27141: Fix some bug in web ui dags list page (auto-refresh & jump search null state)
Posted by GitBox <gi...@apache.org>.
BobDu commented on PR #27141:
URL: https://github.com/apache/airflow/pull/27141#issuecomment-1328055480
@jyotsa09
Oh, this error is beacuse of support search none task in PR #26584 . But only charry-pick this PR in 2.4.3
I think, the only thing we can do now is probably wait 2.5.0 release.
--
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 #27141: Fix some bug in web ui dags list page (auto-refresh & jump search null state)
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #27141:
URL: https://github.com/apache/airflow/pull/27141#discussion_r1000917772
##########
airflow/www/static/js/dags.js:
##########
@@ -255,78 +264,14 @@ function drawDagStatsForDag(dagId, states) {
function dagStatsHandler(error, json) {
Object.keys(json).forEach((dagId) => {
const states = json[dagId];
- drawDagStatsForDag(dagId, states);
+ drawDagAndTaskStatsForDag('dag-run', dagId, states);
});
}
-function drawTaskStatsForDag(dagId, states) {
- const g = d3.select(`svg#task-run-${dagId.replace(/\./g, '__dot__')}`)
- .attr('height', diameter + (strokeWidthHover * 2))
- .attr('width', (states.length * (diameter + circleMargin)) + circleMargin)
- .selectAll('g')
- .data(states)
- .enter()
- .append('g')
- .attr('transform', (d, i) => {
- const x = (i * (diameter + circleMargin)) + (diameter / 2 + circleMargin);
- const y = (diameter / 2) + strokeWidthHover;
- return `translate(${x},${y})`;
- });
-
- g.append('svg:a')
- .attr('href', (d) => `${taskInstanceUrl}?_flt_3_dag_id=${dagId}&_flt_3_state=${d.state}`)
- .append('circle')
- .attr('id', (d) => `task-${dagId.replace(/\./g, '_')}${d.state || 'none'}`)
- .attr('class', 'has-svg-tooltip')
- .attr('stroke-width', (d) => {
- if (d.count > 0) return strokeWidth;
-
- return 1;
- })
- .attr('stroke', (d) => {
- if (d.count > 0) return STATE_COLOR[d.state];
-
- return 'gainsboro';
- })
- .attr('fill', '#fff')
- .attr('r', diameter / 2)
- .attr('title', (d) => d.state || 'none')
- .on('mouseover', function mouseOver(d) {
- if (d.count > 0) {
- d3.select(this).transition().duration(400)
- .attr('fill', '#e2e2e2')
- .style('stroke-width', strokeWidthHover);
- }
- })
- .on('mouseout', function mouseOut(d) {
- if (d.count > 0) {
- d3.select(this).transition().duration(400)
- .attr('fill', '#fff')
- .style('stroke-width', strokeWidth);
- }
- })
- .style('opacity', 0)
- .transition()
- .duration(300)
- .delay((d, i) => i * 50)
- .style('opacity', 1);
-
- d3.select('.js-loading-task-stats').remove();
-
- g.append('text')
- .attr('fill', '#51504f')
- .attr('text-anchor', 'middle')
- .attr('vertical-align', 'middle')
- .attr('font-size', 9)
- .attr('y', 3)
- .style('pointer-events', 'none')
- .text((d) => (d.count > 0 ? d.count : ''));
-}
-
function taskStatsHandler(error, json) {
Object.keys(json).forEach((dagId) => {
const states = json[dagId];
- drawTaskStatsForDag(dagId, states);
+ drawDagAndTaskStatsForDag('task-run', dagId, states);
Review Comment:
Let's make the selectors constants at the top of the file. And probably use task instance vs run, for consistency's sake.
`const TASK_INSTANCE = 'task-instance';`
`const DAG_RUN = 'dag-run';`
--
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 #27141: Fix some bug in web ui dags list page (auto-refresh & jump search null state)
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #27141:
URL: https://github.com/apache/airflow/pull/27141#discussion_r1000914202
##########
airflow/www/static/js/dags.js:
##########
@@ -204,33 +204,41 @@ function drawDagStatsForDag(dagId, states) {
});
g.append('svg:a')
- .attr('href', (d) => `${dagRunUrl}?_flt_3_dag_id=${dagId}&_flt_3_state=${d.state}`)
+ .attr('href', (d) => {
+ const params = new URLSearchParams();
+ params.append('_flt_3_dag_id', dagId);
+ /* eslint no-unused-expressions: ["error", { "allowTernary": true }] */
+ d.state ? params.append('_flt_3_state', d.state) : params.append('_flt_8_state', '');
Review Comment:
```suggestion
params.append('_flt_3_state', d.state || '');
```
I think this would be simpler, no? and then we don't need the eslint exception?
--
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] jyotsa09 commented on pull request #27141: Fix some bug in web ui dags list page (auto-refresh & jump search null state)
Posted by GitBox <gi...@apache.org>.
jyotsa09 commented on PR #27141:
URL: https://github.com/apache/airflow/pull/27141#issuecomment-1312052226
Hi @BobDu When I'm trying to click on `none` task instance state circle it's throwing below error -
```
Something bad has happened.
Airflow is used by many users, and it is very likely that others had similar problems and you can easily find
a solution to your problem.
Consider following these steps:
* gather the relevant information (detailed logs with errors, reproduction steps, details of your deployment)
* find similar issues using:
* [GitHub Discussions](https://github.com/apache/airflow/discussions)
* [GitHub Issues](https://github.com/apache/airflow/issues)
* [Stack Overflow](https://stackoverflow.com/questions/tagged/airflow)
* the usual search engine you use on a daily basis
* if you run Airflow on a Managed Service, consider opening an issue using the service support channels
* if you tried and have difficulty with diagnosing and fixing the problem yourself, consider creating a [bug report](https://github.com/apache/airflow/issues/new/choose).
Make sure however, to include all relevant details and results of your investigation so far.
Python version: 3.9.15
Airflow version: 2.4.3.dev1837+astro.1
Node: d23645e727e9
-------------------------------------------------------------------------------
Error! Please contact server admin.
```
--
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 #27141: Fix some bug in web ui dags list page (auto-refresh & jump search null state)
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #27141:
URL: https://github.com/apache/airflow/pull/27141#discussion_r1000915237
##########
airflow/www/static/js/dags.js:
##########
@@ -189,10 +189,10 @@ d3.selectAll('.js-last-run-tooltip')
d3.select(this).attr('data-original-title', tiTooltip(lastRunData));
});
-function drawDagStatsForDag(dagId, states) {
- const g = d3.select(`svg#dag-run-${dagId.replace(/\./g, '__dot__')}`)
+function drawDagAndTaskStatsForDag(selector, dagId, states) {
Review Comment:
nit. if we're going to rename this, lets make it simpler? `drawDagStats`?
--
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 #27141: Fix some bug in web ui dags list page (auto-refresh & jump search null state)
Posted by GitBox <gi...@apache.org>.
bbovenzi merged PR #27141:
URL: https://github.com/apache/airflow/pull/27141
--
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