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

Add map_index to RenderedTaskInstanceFields #22004

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

ashb
Copy link
Member

@ashb ashb commented Mar 4, 2022

In order to do this "properly" we have also migrated it from using execution_date to run_id in its PK

@ashb
Copy link
Member Author

ashb commented Mar 7, 2022

@uranusjr I'm thinking we should add a mapped_value JSON column here in this migration. When does the mapped value become available?

@ashb ashb force-pushed the rtif-runid-migration branch 5 times, most recently from 2256ebe to b4dd7c6 Compare March 8, 2022 23:01
@uranusjr
Copy link
Member

uranusjr commented Mar 9, 2022

I'm thinking we should add a mapped_value JSON column here in this migration. When does the mapped value become available?

They are populated when XCom is resolved, and should be available when RenderedTaskInstanceFields are written. However, since a RenderedTaskInstanceFields row maps to a TaskInstance row, I think we should simply resolve the mapped values directly instead.

On UI we should also add a marker to indicate a particular field value is mapped, so perhaps add a field to record the field names. Or maybe even this is unnecessary since this information is available on the task object? Or maybe in some scenarios we want to read RenderedTaskInstanceFields without parsing the DAG? (Currently I can only find this being used in /rendered-k8s and /rendered-templates, and in both views we already fetch the original task object and know what fields are mapped anyway.)

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Assuming we don’t need to add mapping info to RenderedTaskInstanceFields (I don’t think we do), this should be good to go.

@ashb
Copy link
Member Author

ashb commented Mar 9, 2022

Or maybe in some scenarios we want to read RenderedTaskInstanceFields without parsing the DAG?

The use case i am thinking of is for showing a table of all the mapped TIs in a detail view in the UI. And mapping can apply to things that aren't marked as templated

@github-actions
Copy link

github-actions bot commented Mar 9, 2022

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 9, 2022
@ashb
Copy link
Member Author

ashb commented Mar 9, 2022

Assuming we don’t need to add mapping info to RenderedTaskInstanceFields (I don’t think we do), this should be good to go.

Even if we do we can alter this migration in a future PR.

In order to do this "properly" we have also migrated it from using
execution_date to run_id in its PK
@uranusjr
Copy link
Member

uranusjr commented Mar 9, 2022

sqlalchemy.exc.OperationalError: (MySQLdb._exceptions.OperationalError) (1025, "Error on rename of './airflow/#sql-1_b' to './airflow/rendered_task_instance_fields' (errno: 150 - Foreign key constraint is incorrectly formed)")

MySQL is not happy…

@ashb
Copy link
Member Author

ashb commented Mar 9, 2022

Okay, I'm confused -- the mysql tests are not behaving like this for me locally Oh downgrade.

@ashb ashb merged commit d6031f9 into apache:main Mar 9, 2022
AIP-42: Dynamic Task Mapping automation moved this from In progress to Done Mar 9, 2022
@ashb ashb deleted the rtif-runid-migration branch March 9, 2022 12:49
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 8, 2022
@jedcunningham jedcunningham added this to the Airflow 2.3.0 milestone Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-39 Timetables area:API Airflow's REST/HTTP API area:dynamic-task-mapping AIP-42 changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge kind:documentation
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants