Skip to content

Switch XCom implementation to use run_id #20975

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.

Sign up for GitHub

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

Jump to bottom
Merged
uranusjr merged 22 commits into from
Feb 16, 2022
Merged

Switch XCom implementation to use run_id #20975

uranusjr merged 22 commits into from
Feb 16, 2022

Conversation

@uranusjr
Copy link
Member

@uranusjr uranusjr commented Jan 20, 2022

Real-world tests needed.

@uranusjr uranusjr requested review from ashb and kaxil January 20, 2022 06:10
@uranusjr uranusjr added this to the Airflow 2.3.0 milestone Jan 20, 2022
@uranusjr

This comment has been minimized.

Sign in to view
@potiuk

This comment has been minimized.

Sign in to view
@uranusjr

This comment has been minimized.

Sign in to view
@uranusjr uranusjr force-pushed the xcom-run-id branch 4 times, most recently from 3d93fe2 to dbc24bb Compare January 24, 2022 13:50
@uranusjr

This comment has been minimized.

Sign in to view
@uranusjr
Copy link
Member Author

uranusjr commented Jan 29, 2022

Nice, MySQL is now failing on the same places as Postgres. SQLite fails just a few more (it’s complaining about unique constraint violation not happening on other backends, not sure why). MSSQL is complaining about the key in x.key being a reserved word; I guess I need to find a good way to quote it.

@ashb

This comment has been minimized.

Sign in to view
@uranusjr

This comment has been minimized.

Sign in to view
@ashb

This comment has been minimized.

Sign in to view
@uranusjr uranusjr force-pushed the xcom-run-id branch 2 times, most recently from df2133a to 3e28291 Compare February 1, 2022 11:36
@uranusjr

This comment has been minimized.

Sign in to view
@uranusjr uranusjr force-pushed the xcom-run-id branch 3 times, most recently from 60e419d to 54e6cb7 Compare February 3, 2022 13:46
@uranusjr
Copy link
Member Author

uranusjr commented Feb 3, 2022

Marking this as ready since all failing tests are from main. This does not break anything.

@uranusjr uranusjr marked this pull request as ready for review February 3, 2022 14:26
@ashb
Copy link
Member

ashb commented Feb 15, 2022

Static checks are fixed in main.

@uranusjr
Switch XCom implementation to use run_id
45c3a87
@uranusjr
Check dangling XCom on upgrade
cf50c59
@uranusjr
Fix XCom.set usage in tests
30da9c4
@uranusjr
Try to not use batch op
53ef8d9
@uranusjr
Fix migration for non-Postgres
a93abc8
@uranusjr
Fix _create_xcom_entries to use run_id
7fba350
@uranusjr
Fix deprecated call in TaskInstance.clear_xcom_data()
836ce87
@uranusjr
Split XCom pkey into multiple indexes
cade177 
This works around MySQL's index size limit.
@uranusjr
Use (DagRun.id, XCom.key) as XCom primary key
f4ad039 
The pk that makes the most sense, (key, dag_id, task_id, run_id), goes
over MySQL's index length limit, so we need so find an alternative way
to enforce uniqueness. DagRun.id replaces (dag_id, run_id) and allows us
to stay under the limit.
@uranusjr
Replace XCom.execution_date usages with run_id
b83aaa5
@uranusjr
Fix deserializing from None
81cec6a 
I don't know why it worked previously, but I know it works now.
@uranusjr
Convert XComSchema tests to pytest
f9ecad8
@uranusjr
Fix XCom fetch with execution_date
5902bdc
@uranusjr
Fix KubernetesPodOperator xcom push test
2234424
@uranusjr
Use SQLAlchemy expressions for insert in migration
7ed19bf 
This allows the query to make use of SQLAlchemy's keyword quoting logic
so we can correctly handle quote differences between different database
implementations (I'm looking at you, MySQL and Microsoft SQL Server).
@uranusjr
Fix XCom API tests
c09c880
@uranusjr
Add special case setting XCom for in-memory DAG run
e7b6158
@uranusjr
Properly set up DAG run for XCom push in get_link test
9ed8cc6
@uranusjr
Migrate XCom model view column declaration
eb49136
@uranusjr
Clean up XCom in some tests
3e39f11 
This seems to be needed now.
@uranusjr
Add upgrade note on XCom.execution_date
bd8f102
@uranusjr
Create index after populating all xcom data
69a5766
@uranusjr uranusjr merged commit 0ebd642 into apache:main Feb 16, 2022
@uranusjr uranusjr deleted the xcom-run-id branch February 16, 2022 02:47
@alexott
Copy link
Contributor

alexott commented Feb 20, 2022

This change lead to failing migrations because I had several DAGs with the same task name, but different DAG names. Should be xcom results be identified by DAG name as well?

@jedcunningham jedcunningham added the type:improvement Changelog: Improvements label Feb 28, 2022
@ashb ashb mentioned this pull request Mar 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ephraimbuddy ephraimbuddy ephraimbuddy left review comments

@ashb ashb ashb approved these changes

@potiuk potiuk potiuk approved these changes

@kaxil kaxil Awaiting requested review from kaxil

@bbovenzi bbovenzi Awaiting requested review from bbovenzi

@mik-laj mik-laj Awaiting requested review from mik-laj

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton

@XD-DENG XD-DENG Awaiting requested review from XD-DENG

Assignees

No one assigned

Labels

AIP-39 Timetables area:dynamic-task-mapping AIP-42 full tests needed We need to run full set of tests for this PR to merge type:improvement Changelog: Improvements

Projects

None yet

Milestone

Airflow 2.3.0

Development

Successfully merging this pull request may close these issues.

6 participants

@uranusjr @potiuk @ashb @jedcunningham @alexott @ephraimbuddy