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

Remove API-44 methods from method map #44494

Merged
merged 1 commit into from
Nov 30, 2024

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Nov 30, 2024

The changes #44490, #44491, #44489 removed internal_api_call decorator from a few methods but did not remove them from method map.

This PR is a follow-up.


^ 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.

The changes apache#44490, apache#44491, apache#44489 removed internal_api_call
decorator from a few methods but did not remove them from method map.

This PR is a follow-up.
@potiuk potiuk merged commit 84907f1 into apache:main Nov 30, 2024
65 checks passed
@potiuk potiuk deleted the remove-removed-internal-api-calls branch November 30, 2024 00:17
@jscheffl
Copy link
Contributor

Hi @potiuk While I like the cleanup and reversion of AIP-44 you are running faster than I can make the other PR to main to move EdgeExecutor off the AIP-44.

The method map is broken on main and for Airflow 3 can be completely removed. BUT we need it further for the backcompat case in Airflow 2.10. There I need to restore the state of Airflow 2.10, else it is both broken in Airflow 2.10 as well as in Airflow 3.

While I accept it is temporarily broken in Airflow 3 (Still running behind trying to make EdgeExecutor working on main (again after the stone was hitting the glass) with the ongoing changes and merges to EdgeExecutor it is also throwing stones on the ability to further use it in Airflow 2.10.

@potiuk
Copy link
Member Author

potiuk commented Nov 30, 2024

While I accept it is temporarily broken in Airflow 3 (Still running behind trying to make EdgeExecutor working on main (again after the stone was hitting the glass) with the ongoing changes and merges to EdgeExecutor it is also throwing stones on the ability to further use it in Airflow 2.10.

I think we should not hold back the changes for Airflow 2.10 compatibilty. This is the main thing we discussed before that it will have to end at some point of time - when it starts blocking our Airflow 3 development, And I think this is the time.

I'd say - if we need to have new features of edge executor to work for 2.10 (particularly in your installation - I doubt it is used by anyone else) then maybe you should fork airflow right before we started to remove AIP-44 stuff, and continue to work on changes in standard executor that will be 2.10 compatible / backport there - and build it and use it internally.

I think that was inevitable and there is no better way - there is no easy way same edge executor will work for both 2.10 and 3 becasue the APIs will be so vastly different (and AIP-44 completely removed in Airflow 3).

I'd say that this is the right moment to break the compatibility and let the burden on maintaining 2.10 variant of the edge executor to stay in the hands of Bosh, rather than keep us from cleaning and developing Airflow 3.

@kaxil @ashb - I guess the AIP-44 code is keeping you from AIP-72 progressing (it looked like from the discussions we had so far and the plans for next week to make PRs that are overlapping with the current AIP-44 cleanup.

We can raise it as a discussion at the devlist as well, but I think that was part of the agreement that we keep AIP-44 and Edge Executor happy with it for as long as we can - and I think "as long as we can" is "NOW".

@potiuk
Copy link
Member Author

potiuk commented Nov 30, 2024

Also technically speaking- when it comes to the mechanics of it.

That should be of course a bit of a burden for you and your team - but this is completely no problem to fork Airflow and run breeze prepare-proivders-package edge from it - update versions there, test it etc.

So this is just a matter of backporting any changes in main edge executor to that fork to make it works with Airflow 2.10 / AIP-44.

You can also easily run the same compatibility tests in your CI - using your fork, so technically speaking you could do everything to release next "2.10" compatible edge worker on your own - without having to keep the compatibility in Airflow repo.

So all that is doable - the question is - should it slow down the current Airflow 3 development, or should it slow down Bosh's team runnning the 2.10 version of it. i think that's basiclly the choice we are facing now.

@ashb
Copy link
Member

ashb commented Nov 30, 2024

@kaxil @ashb - I guess the AIP-44 code is keeping you from AIP-72 progressing

Not especially, actually. It's very occasionally a mild inconvenience but not a blocker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:edge Edge Executor / Worker (AIP-69)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants