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

Avoid color info in response of /dag_stats & /task_stats #8742

Merged
merged 3 commits into from
May 11, 2020

Conversation

XD-DENG
Copy link
Member

@XD-DENG XD-DENG commented May 6, 2020

Currently the color for each state is repeatedly appearing in the response payload of endpoints /dag_stats and /task_stats.

This can be avoided by having the mapping between state and color in the static file, and map each state into different color at client side after client receives the necessary info, instead of passing duplicated color information in the response payload.

This significantly reduces the size of data to be transferred from server to client.


Make sure to mark the boxes below before creating PR: [x]


In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label May 6, 2020
@@ -337,6 +337,9 @@ def get_int_arg(value, default=0):

num_of_pages = int(math.ceil(num_of_all_dags / float(dags_per_page)))

state_color_mapping = State.state_color.copy()
state_color_mapping["null"] = state_color_mapping.pop(None)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None is one of the keys in State.state_color, and it cannot be handled by Flask's tojson directly. So a special treatment is needed here.

A related issue I raised in Flask: pallets/flask#3599

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keys in JSON must be strings, so { null: "x"} isn't valid JSON. So it sort of makes sense that it doesn't get added. Sort of.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python already handles it for us:

import json
print(json.dumps({None: 123}))

gives us {"null": 123}.

The issue is the sort_keys argument when we run json.dumps().

@XD-DENG XD-DENG force-pushed the feature/avoid-repeated-color-data branch from 2969535 to afd2136 Compare May 6, 2020 19:35
@@ -353,6 +356,7 @@ def get_int_arg(value, default=0):
status=arg_status_filter if arg_status_filter else None),
num_runs=num_runs,
tags=tags,
state_color=state_color_mapping,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. Would you like to add a test that will check if the correct value was passed to the template? A method has been added today that makes it easy. I think it's worth gradually extending this idea to other tests to increase the confidence of the Web UI tests.
#8505

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mik-laj , thanks for the suggestion.

I have done an update by adding an assert in an existing test, which should suffice the purpose here.

Let me know if you have any other suggestion? Cheers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mik-laj , I have further updated the UI test code using the method you shared. Please take a look when you get time.

Two tests are failing, https://travis-ci.org/github/apache/airflow/jobs/684134650#L16198 and https://github.com/apache/airflow/pull/8742/checks?check_run_id=652054298 . But both seem interim and irrelevant to the changes of this PR

@XD-DENG XD-DENG requested review from mik-laj, ashb and kaxil May 6, 2020 20:38
@XD-DENG XD-DENG force-pushed the feature/avoid-repeated-color-data branch from 6700ae4 to 071a742 Compare May 7, 2020 16:18
@XD-DENG
Copy link
Member Author

XD-DENG commented May 7, 2020

Rebased to the latest master

Currently the color for each state is repeatedly appearing
in the response payload of endpoints /dag_stats and /task_stats.

This can be avoided by having the mapping between state and color in the
static file, and map each state into different color at client side after
client receives the necessary info, instead of passing duplicated
color information in the response payload.

This significantly reduces the size of data to be transferred from
server to client.
@XD-DENG XD-DENG force-pushed the feature/avoid-repeated-color-data branch from 071a742 to 390282b Compare May 10, 2020 16:33
@XD-DENG
Copy link
Member Author

XD-DENG commented May 10, 2020

Hi guys @mik-laj @kaxil @ashb , a gentle ping. Mind taking another look?

@XD-DENG XD-DENG added this to the Airflow 1.10.11 milestone May 11, 2020
@XD-DENG XD-DENG merged commit bed1995 into apache:master May 11, 2020
kaxil pushed a commit that referenced this pull request Jun 26, 2020
* Avoid color info in response of /dag_stats & /task_stats

Currently the color for each state is repeatedly appearing
in the response payload of endpoints /dag_stats and /task_stats.

This can be avoided by having the mapping between state and color in the
static file, and map each state into different color at client side after
client receives the necessary info, instead of passing duplicated
color information in the response payload.

This significantly reduces the size of data to be transferred from
server to client.

(cherry-picked from bed1995)
potiuk pushed a commit that referenced this pull request Jun 29, 2020
* Avoid color info in response of /dag_stats & /task_stats

Currently the color for each state is repeatedly appearing
in the response payload of endpoints /dag_stats and /task_stats.

This can be avoided by having the mapping between state and color in the
static file, and map each state into different color at client side after
client receives the necessary info, instead of passing duplicated
color information in the response payload.

This significantly reduces the size of data to be transferred from
server to client.

(cherry-picked from bed1995)
@kaxil kaxil added the type:improvement Changelog: Improvements label Jul 1, 2020
kaxil pushed a commit that referenced this pull request Jul 1, 2020
* Avoid color info in response of /dag_stats & /task_stats

Currently the color for each state is repeatedly appearing
in the response payload of endpoints /dag_stats and /task_stats.

This can be avoided by having the mapping between state and color in the
static file, and map each state into different color at client side after
client receives the necessary info, instead of passing duplicated
color information in the response payload.

This significantly reduces the size of data to be transferred from
server to client.

(cherry-picked from bed1995)
@XD-DENG XD-DENG deleted the feature/avoid-repeated-color-data branch August 16, 2020 08:24
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
* Avoid color info in response of /dag_stats & /task_stats

Currently the color for each state is repeatedly appearing
in the response payload of endpoints /dag_stats and /task_stats.

This can be avoided by having the mapping between state and color in the
static file, and map each state into different color at client side after
client receives the necessary info, instead of passing duplicated
color information in the response payload.

This significantly reduces the size of data to be transferred from
server to client.

(cherry-picked from bed1995)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:performance area:webserver Webserver related Issues type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants