potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751711095
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
> The following change should handle airflow<2.3 -- it checks the
function signature and see if it has context in it or no
Yeah. I understand that. I am thinking more about implementation of
operators in providers. If people will start taking advantage of the context,
their implementations might start (even accidentally) depend on context being
available to perform "proper" cleanup. I think in pretty much all cases this
will be the case (why else would you use it?). Either you use context to do the
proper cleanup, or you leave stuff behind. If the implemetnation "depends" on
the on_kill having context it might make those future operators provide poor
experience on < 2.3.0 even accidentally.
I see why we want to implement it - and I am not at all opposing it, I am
just thinking how to "make sure" we have some protections so that somoene who
did not yet migrate to 2.3.0 will use providers released after this change
(that might rely on the context being present) and got memory/pod leaks etc..
I thought a bit on that and another option is to "force" Airflow >= 2.3.0"
for any provider that declares 'context' in on_kill - this can be automateded
with pre-commit check. This way we add the >= 2.3.0 limitation only for those
providers that use the context.
For example the first time we implement `on_kill(context)` in
KubernetesPodOperator, we add `apache-airflow >= 2.3.0`. - so that it will not
be usable in 2.2.2 for example
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
