-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add create XCom endpoint in RestAPI #46042
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
Conversation
|
Just adding comments as copy of the 1:1 conversation: I would understand if the API can be used to "patch" wrong/incorrect XCom values administratively for specific szenarios where upstream tasks can not be re-executed but values are incorrect. But the PR of this API is only adding XCom, will raise an error if the value is existing. So before merging I'd propose to have a bit of discussion between the other maintainers if we want to open XCom up to external applications to signal a processing status. If so then this integration pattern would require a bit of documentation as well. |
This is not the use case. The use case is incident recovery. When on call fixes problems with data pipeline manual interventions sometimes required. |
If I read the PR description incident recovery is not the use case. For incident recovery also patching of XCom would be needed. It is called as:
|
|
@jscheffl The PR is to solve the pain I raised in #45966 @shubhamraj-git I suggest to strike trough the extra use cases mentioned in the description. |
jscheffl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are missing
Yes, I was waiting for discussions to be done. I will include the tests before merging. @jscheffl |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me.
Indeed we need tests before being able to merge it.
Same as Jens on this one, indeed it feels a little weird from a functional point of view, but if this is required for some use case, then I'm fine with it.
|
ready to merge? |
|
Yes, added the tests and good to merge. |
|
@pierrejeambrun @jscheffl any further comments? |
No. No objections. |
|
I think we need one last rebase. |
* create xcom api * Add tests for create xcom API * Small tweak --------- Co-authored-by: pierrejeambrun <pierrejbrun@gmail.com>
* create xcom api * Add tests for create xcom API * Small tweak --------- Co-authored-by: pierrejeambrun <pierrejbrun@gmail.com>
related: #45966
Update: Modifying the description only for create API and simplifying use case.
The XCom creation API fills a critical gap by enabling dynamic updates to workflows.
Current XComs are tied to task execution, limiting flexibility for long-running workflows.
The API allows dynamic creation of XCom values during execution, This is needed mostly for the use case of fixing manually recover from problems with workflows.
Avoid placeholder tasks just to generate XComs.
We will also have the edit XCom API as part of #45966 coming soon. This PR is only for creation.
Steps to play around the feature.
This Airflow DAG demonstrates how to use XComs for passing data between tasks. The first task (wait_and_not_push) waits for 1 minute but does not push any XCom. The second task (pull_and_print) attempts to pull an XCom value with the key outbound_key1 from the first task, logs it if found, or warns if absent.
Now, check the logs, You can see the XCom pull was successful.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.