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

KPO Maintain backward compatibility for execute_complete and trigger run method #37363

Merged
merged 9 commits into from
Feb 14, 2024

Conversation

pankajastro
Copy link
Member

@pankajastro pankajastro commented Feb 12, 2024

In #37279 I introduce periodic logging of the container.
During the process, I also changed a few event Dict key names
and that is problematic for someone extending the KPO trigger.
Also, the current execute_compelete method was unused in the KPO operator
and was problematic if someone using it in an extended class since
now the trigger can also emit an event even if the pod is in the pod intermediate state.
one reported issue: #37279 (comment)
In this PR I'm restoring the trigger event dict structure.
Also, deprecating the execute_complete method


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues labels Feb 12, 2024
@pankajastro pankajastro changed the title Restore KPO trigger event param name KPO: Maintain backward compatibility for execute_complete and trigger run method Feb 14, 2024
@pankajastro pankajastro changed the title KPO: Maintain backward compatibility for execute_complete and trigger run method KPO Maintain backward compatibility for execute_complete and trigger run method Feb 14, 2024
@pankajastro pankajastro marked this pull request as ready for review February 14, 2024 14:53
@vchiapaikeo
Copy link
Contributor

Hi @pankajastro , I tested the same snippet that I tested here: #37279 (comment)

And this time it worked! 🎉

Thank you for the quick fix!

@pankajkoti
Copy link
Member

Thank you so much @vchiapaikeo for quickly testing this out

@eladkal eladkal merged commit 0640e6d into apache:main Feb 14, 2024
63 checks passed
@pankajastro pankajastro deleted the fix_kpo branch February 14, 2024 19:14
potiuk added a commit to potiuk/airflow that referenced this pull request Feb 15, 2024
potiuk added a commit that referenced this pull request Feb 15, 2024
sunank200 pushed a commit to astronomer/airflow that referenced this pull request Feb 21, 2024
…run method (apache#37363)

* Restore KPO trigger event param name

* sync with execute_complete

* change trigger emit state done to failed/scuccess

* Add deprecation warning

* Address review feedback

* Fix tests and cleanup

* Fix tests

* Remove test log stmpt

* Fix tests

---------

Co-authored-by: Pankaj Koti <pankajkoti699@gmail.com>
sunank200 pushed a commit to astronomer/airflow that referenced this pull request Feb 21, 2024
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
…run method (apache#37363)

* Restore KPO trigger event param name

* sync with execute_complete

* change trigger emit state done to failed/scuccess

* Add deprecation warning

* Address review feedback

* Fix tests and cleanup

* Fix tests

* Remove test log stmpt

* Fix tests

---------

Co-authored-by: Pankaj Koti <pankajkoti699@gmail.com>
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants