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

Fix DB session handling in XCom.set. #18240

Merged
merged 1 commit into from
Sep 15, 2021

Conversation

ashb
Copy link
Member

@ashb ashb commented Sep 14, 2021

Since the function has the @provide_session decorator it should not be
committing the session (the decorator handles that if it's not passed a
session) and nor should it be calling expunge_all -- that detaches all
objects from the session which is just not needed (or right) behaviour
form setting an XCom value.

By using the session fixture we get the transaction automatically
rolled back, so we don't need any setup/teardown methods.

(This is a prep PR for fixing the deserialize behavoiur of XCom.get_one)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

@ashb ashb added this to the Airflow 2.2 milestone Sep 14, 2021
@ashb ashb requested review from uranusjr and kaxil September 14, 2021 12:59
@ashb ashb requested a review from XD-DENG as a code owner September 14, 2021 12:59
Since the function has the `@provide_session` decorator it should not be
committing the session (the decorator handles that if it's not passed a
session) and nor should it be calling expunge_all -- that detaches all
objects from the session which is just not needed (or right) behaviour
form setting an XCom value.

By using the `session` fixture we get the transaction automatically
rolled back, so we don't need any setup/teardown methods
@uranusjr
Copy link
Member

Are even the flushes needed?

@github-actions
Copy link

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 Sep 15, 2021
@ashb
Copy link
Member Author

ashb commented Sep 15, 2021

Are even the flushes needed?

Possibly not striclty needed, but its hard to reason about where this might be used, so a flush is safer

@uranusjr uranusjr closed this Sep 15, 2021
@uranusjr uranusjr reopened this Sep 15, 2021
@ashb ashb merged commit 902aeb7 into apache:main Sep 15, 2021
@ashb ashb deleted the fix-xcom-sesison-handling branch September 15, 2021 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants