[Mothership PR] Remove internal_api_call decorator

[shahar1]

related: #44436

---

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

[vincbeck]

As part of this task, #44436 mentions as well "re-join back methods that were separated out from the main code - when methods start with _". Are you planning to do it in this PR? I'd advice to either do it as part of this PR or do the re-join before removing the decorator. Once the decorator is removed, it will be harder to look for methods that were used to be used for AIP-44 and needs to be re-joined

[shahar1]

As part of this task, #44436 mentions as well "re-join back methods that were separated out from the main code - when methods start with _". Are you planning to do it in this PR? I'd advice to either do it as part of this PR or do the re-join before removing the decorator. Once the decorator is removed, it will be harder to look for methods that were used to be used for AIP-44 and needs to be re-joined

Yes, that's why I just drafted it :)
I'll take care of it, but just to make sure that I understand "re-join" correctly - everywhere where there's both def fun() and def _fun(), simply restore the logic from _fun() to fun(), or is it deeper than that.
Also:

1. What should I do if _fun() is called more than once like ./airflow/dag_processing/processor.py: _execute_task_callbacks?
2. What should we do about these functions in rcp_api.py? Rename them to their original?

It's my first time in the API area, so I'll try not to break too many things :)

[vincbeck]

Yes, that's why I just drafted it :)

Cool :)

I'll take care of it, but just to make sure that I understand "re-join" correctly - everywhere where there's both def fun() and def _fun(), simply restore the logic from _fun() to fun(), or is it deeper than that?

Yes you understood well what re-join means. When working on AIP-44, many methods were moved to private functions to make them "callable". You'll see many methods like:

@classmethod
@internal_api_call
def function_name(self, ...)
  return _function_name(...)

The goal is to move back the implementation of _function_name into function_name.

Also, what should I do if _fun() is called more than once like ./airflow/dag_processing/processor.py: _execute_task_callbacks? (it's my first time in the API area)

In this case, (please @potiuk correct me if I am wrong) I dont think we should re-join this method. _execute_task_callbacks is called in different methods and the separation makes sense.

What should we do about these functions in rcp_api.py? Rename them to their original?
Are you talking about airflow/api_internal/endpoints/rpc_api_endpoint.py? This file has been removed recently, so no need to take care of it

It's my first time in the API area, so I'll try not to break too many things :)

No worries! Thanks for doing it! We all have to start someday right?

[shahar1]

Yes, that's why I just drafted it :)

Cool :)

I'll take care of it, but just to make sure that I understand "re-join" correctly - everywhere where there's both def fun() and def _fun(), simply restore the logic from _fun() to fun(), or is it deeper than that?

Yes you understood well what re-join means. When working on AIP-44, many methods were moved to private functions to make them "callable". You'll see many methods like:

@classmethod
@internal_api_call
def function_name(self, ...)
  return _function_name(...)

The goal is to move back the implementation of _function_name into function_name.

Also, what should I do if _fun() is called more than once like ./airflow/dag_processing/processor.py: _execute_task_callbacks? (it's my first time in the API area)

In this case, (please @potiuk correct me if I am wrong) I dont think we should re-join this method. _execute_task_callbacks is called in different methods and the separation makes sense.

What should we do about these functions in rcp_api.py? Rename them to their original?
Are you talking about airflow/api_internal/endpoints/rpc_api_endpoint.py? This file has been removed recently, so no need to take care of it

It's my first time in the API area, so I'll try not to break too many things :)

No worries! Thanks for doing it! We all have to start someday right?

Thanks for the deatiled response, I'm on it!
I'm talking about providers/src/airflow/providers/edge/worker_api/routes/rpc_api.py (maybe @jscheffl could help with this one :) )

[potiuk]

In this case, (please @potiuk correct me if I am wrong) I dont think we should re-join this method. _execute_task_callbacks is called in different methods and the separation makes sense.

Yes. We can maybe even nicely refactor it to make more sense and rename such methods, place them in the "right" place etc. This should be case-by-case decision and that's why in #44436 I extracted all the method lists so that we can do it in pieces - maybe not one-by-one, but maybe file-by-file, so that we could review the change in a meaningful way.

There are, for example, some cases where likely after re-joining we will be able to undo some of the changes made (for example there were few methods where we artifficially had to add commit() where otherwise it was not needed - because the logic was split into "client" and "server" in a sequence of transctions. By having smaller, focused commits "per-file" we can look at the historical changes to it and when we extracted it and decide what to do.

Also splitting it "per-group" might be easily distributed between different people in this case.

[potiuk]

And yes. Cool you are looking at it @shahar1 -> i think a good idea will be by anyone who starts looking at removing the method from one of those files announce "I am taking this part" in #446436 in order to avoid duplication :)

[potiuk]

Then - this one can be rebased gradually until we get to "no change" :D

[potiuk]

I have realized that we forgot to remove method maps: #44494
And here is an example of "joining" back the methods that is more than just joining @shahar1 #44493