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

Remove useless statement in task_group_to_grid #25654

Conversation

pierrejeambrun
Copy link
Member

@pierrejeambrun pierrejeambrun commented Aug 10, 2022

This PR just removes a statement that had no effect.

We keep sending possibly 'None' state in the grid data except for the 'mapped_states' count. (None keys in json are an issue)

I have another version of this PR here, where instead, None status are changed to 'no_status' in the entire returned payload. I don't think this is a good idea because either way the front will still have to handle None state. (Other endpoints returning TaskInstance will not do this transformation).

I plan to work on grid_data endpoint to make it return object following the api_connexion.TaskInstance schema. (run_id vs dag_run_id and other differences), to be able to unify those types in the front. (They are not playing nice with each other atm)

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Aug 10, 2022
@ashb
Copy link
Member

ashb commented Aug 10, 2022

I plan to work on grid_data endpoint follow the api_connexion.TaskInstance schema. (run_id vs dag_run_id and other differences), to be able to unify those types in the front. (They are not playing nice with each other right now)

So the grid_data endpoint is "private"/"internal" which means the format of the data it returns should be tailored to exactly what the UI needs, and not generic like the OpenAPI REST API. So only change the format if it makes the UI cleaner (don't just do it for the sake of it in other words)

@bbovenzi
Copy link
Contributor

bbovenzi commented Aug 10, 2022

So the grid_data endpoint is "private"/"internal" which means the format of the data it returns should be tailored to exactly what the UI needs, and not generic like the OpenAPI REST API. So only change the format if it makes the UI cleaner (don't just do it for the sake of it in other words)

Yeah, that's the plan. There's some inconsistencies between the task instances in the internal grid_data and the REST get_mapped_instances. This causes some issues in the UI code. That's what Pierre wants to fix.

@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Aug 10, 2022

Understood @ashb, thanks for the details :). Yes the main reason here is to avoid manual object conversion and casting in the UI code.

@bbovenzi bbovenzi merged commit 84718f9 into apache:main Aug 10, 2022
@pierrejeambrun pierrejeambrun deleted the remove-useless-statement-in-task-group-to-grid branch August 10, 2022 22:08
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Aug 15, 2022
@ephraimbuddy ephraimbuddy added this to the Airflow 2.4.0 milestone Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants